[PATCH 1/2] fbcon: Register sysfs groups through device_add_group

[PATCH 1/2] fbcon: Register sysfs groups through device_add_group

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

Use device_add_group() to simplify creation and removal.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/video/fbdev/core/fbcon.c | 48 +++++++++++++++-----------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 07d127110ca4..51c3e8a5a092 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -3159,7 +3159,7 @@ static const struct consw fb_con = {
 	.con_debug_leave	= fbcon_debug_leave,
 };
 
-static ssize_t store_rotate(struct device *device,
+static ssize_t rotate_store(struct device *device,
 			    struct device_attribute *attr, const char *buf,
 			    size_t count)
 {
@@ -3181,7 +3181,7 @@ static ssize_t store_rotate(struct device *device,
 	return count;
 }
 
-static ssize_t store_rotate_all(struct device *device,
+static ssize_t rotate_all_store(struct device *device,
 				struct device_attribute *attr,const char *buf,
 				size_t count)
 {
@@ -3203,7 +3203,7 @@ static ssize_t store_rotate_all(struct device *device,
 	return count;
 }
 
-static ssize_t show_rotate(struct device *device,
+static ssize_t rotate_show(struct device *device,
 			   struct device_attribute *attr,char *buf)
 {
 	struct fb_info *info;
@@ -3222,7 +3222,7 @@ static ssize_t show_rotate(struct device *device,
 	return sysfs_emit(buf, "%d\n", rotate);
 }
 
-static ssize_t show_cursor_blink(struct device *device,
+static ssize_t cursor_blink_show(struct device *device,
 				 struct device_attribute *attr, char *buf)
 {
 	struct fb_info *info;
@@ -3247,7 +3247,7 @@ static ssize_t show_cursor_blink(struct device *device,
 	return sysfs_emit(buf, "%d\n", blink);
 }
 
-static ssize_t store_cursor_blink(struct device *device,
+static ssize_t cursor_blink_store(struct device *device,
 				  struct device_attribute *attr,
 				  const char *buf, size_t count)
 {
@@ -3281,32 +3281,30 @@ static ssize_t store_cursor_blink(struct device *device,
 	return count;
 }
 
-static struct device_attribute device_attrs[] = {
-	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
-	__ATTR(rotate_all, S_IWUSR, NULL, store_rotate_all),
-	__ATTR(cursor_blink, S_IRUGO|S_IWUSR, show_cursor_blink,
-	       store_cursor_blink),
+static DEVICE_ATTR_RW(rotate);
+static DEVICE_ATTR_WO(rotate_all);
+static DEVICE_ATTR_RW(cursor_blink);
+
+static struct attribute *fbcon_device_attrs[] = {
+	&dev_attr_rotate.attr,
+	&dev_attr_rotate_all.attr,
+	&dev_attr_cursor_blink.attr,
+	NULL,
+};
+
+static const struct attribute_group fbcon_device_attr_group = {
+	.attrs		= fbcon_device_attrs,
 };
 
 static int fbcon_init_device(void)
 {
-	int i, error = 0;
+	int ret;
 
 	fbcon_has_sysfs = 1;
 
-	for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
-		error = device_create_file(fbcon_device, &device_attrs[i]);
-
-		if (error)
-			break;
-	}
-
-	if (error) {
-		while (--i >= 0)
-			device_remove_file(fbcon_device, &device_attrs[i]);
-
+	ret = device_add_group(fbcon_device, &fbcon_device_attr_group);
+	if (ret)
 		fbcon_has_sysfs = 0;
-	}
 
 	return 0;
 }
@@ -3389,11 +3387,9 @@ void __init fb_console_init(void)
 
 static void __exit fbcon_deinit_device(void)
 {
-	int i;
 
 	if (fbcon_has_sysfs) {
-		for (i = 0; i < ARRAY_SIZE(device_attrs); i++)
-			device_remove_file(fbcon_device, &device_attrs[i]);
+		device_remove_group(fb_info->dev, &fbcon_device_attr_group);
 
 		fbcon_has_sysfs = 0;
 	}
-- 
2.25.1

[PATCH 2/2] fbcon: Change return value type to void

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

fbcon_init_device() doesn't need to return a value.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/video/fbdev/core/fbcon.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 51c3e8a5a092..c1692b68f69e 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -3296,7 +3296,7 @@ static const struct attribute_group fbcon_device_attr_group = {
 	.attrs		= fbcon_device_attrs,
 };
 
-static int fbcon_init_device(void)
+static void fbcon_init_device(void)
 {
 	int ret;
 
@@ -3305,8 +3305,6 @@ static int fbcon_init_device(void)
 	ret = device_add_group(fbcon_device, &fbcon_device_attr_group);
 	if (ret)
 		fbcon_has_sysfs = 0;
-
-	return 0;
 }
 
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
-- 
2.25.1

Re: [PATCH 1/2] fbcon: Register sysfs groups through device_add_group

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.14-rc6 next-20250311]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/oushixiong1025-163-com/fbcon-Change-return-value-type-to-void/20250311-193230
base:   linus/master
patch link:    https://lore.kernel.org/r/20250311112856.1020095-1-oushixiong1025%40163.com
patch subject: [PATCH 1/2] fbcon: Register sysfs groups through device_add_group
config: sparc-randconfig-001-20250312 (https://download.01.org/0day-ci/archive/20250312/202503121852.0x6J0c7a-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250312/202503121852.0x6J0c7a-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503121852.0x6J0c7a-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/video/fbdev/core/fbcon.c: In function 'fbcon_deinit_device':
>> drivers/video/fbdev/core/fbcon.c:3390:37: error: 'fb_info' undeclared (first use in this function)
    3390 |                 device_remove_group(fb_info->dev, &fbcon_device_attr_group);
         |                                     ^~~~~~~
   drivers/video/fbdev/core/fbcon.c:3390:37: note: each undeclared identifier is reported only once for each function it appears in


vim +/fb_info +3390 drivers/video/fbdev/core/fbcon.c

  3388	
  3389		if (fbcon_has_sysfs) {
> 3390			device_remove_group(fb_info->dev, &fbcon_device_attr_group);
  3391	
  3392			fbcon_has_sysfs = 0;
  3393		}
  3394	}
  3395	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] fbcon: Register sysfs groups through device_add_group

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.14-rc6 next-20250312]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/oushixiong1025-163-com/fbcon-Change-return-value-type-to-void/20250311-193230
base:   linus/master
patch link:    https://lore.kernel.org/r/20250311112856.1020095-1-oushixiong1025%40163.com
patch subject: [PATCH 1/2] fbcon: Register sysfs groups through device_add_group
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20250312/202503122323.h75SQfrd-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project e15545cad8297ec7555f26e5ae74a9f0511203e7)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250312/202503122323.h75SQfrd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503122323.h75SQfrd-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/video/fbdev/core/fbcon.c:3390:23: error: use of undeclared identifier 'fb_info'
    3390 |                 device_remove_group(fb_info->dev, &fbcon_device_attr_group);
         |                                     ^
   1 error generated.


vim +/fb_info +3390 drivers/video/fbdev/core/fbcon.c

  3388	
  3389		if (fbcon_has_sysfs) {
> 3390			device_remove_group(fb_info->dev, &fbcon_device_attr_group);
  3391	
  3392			fbcon_has_sysfs = 0;
  3393		}
  3394	}
  3395	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] fbcon: Register sysfs groups through device_add_group

[Thomas Weißschuh]

Hi,

On Tue, Mar 11, 2025 at 07:28:55PM +0800, oushixiong1025@163.com wrote:
> From: Shixiong Ou <oushixiong@kylinos.cn>
> 
> Use device_add_group() to simplify creation and removal.
> 
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
>  drivers/video/fbdev/core/fbcon.c | 48 +++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 07d127110ca4..51c3e8a5a092 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -3159,7 +3159,7 @@ static const struct consw fb_con = {
>  	.con_debug_leave	= fbcon_debug_leave,
>  };
>  
> -static ssize_t store_rotate(struct device *device,
> +static ssize_t rotate_store(struct device *device,
>  			    struct device_attribute *attr, const char *buf,
>  			    size_t count)
>  {
> @@ -3181,7 +3181,7 @@ static ssize_t store_rotate(struct device *device,
>  	return count;
>  }
>  
> -static ssize_t store_rotate_all(struct device *device,
> +static ssize_t rotate_all_store(struct device *device,
>  				struct device_attribute *attr,const char *buf,
>  				size_t count)
>  {
> @@ -3203,7 +3203,7 @@ static ssize_t store_rotate_all(struct device *device,
>  	return count;
>  }
>  
> -static ssize_t show_rotate(struct device *device,
> +static ssize_t rotate_show(struct device *device,
>  			   struct device_attribute *attr,char *buf)
>  {
>  	struct fb_info *info;
> @@ -3222,7 +3222,7 @@ static ssize_t show_rotate(struct device *device,
>  	return sysfs_emit(buf, "%d\n", rotate);
>  }
>  
> -static ssize_t show_cursor_blink(struct device *device,
> +static ssize_t cursor_blink_show(struct device *device,
>  				 struct device_attribute *attr, char *buf)
>  {
>  	struct fb_info *info;
> @@ -3247,7 +3247,7 @@ static ssize_t show_cursor_blink(struct device *device,
>  	return sysfs_emit(buf, "%d\n", blink);
>  }
>  
> -static ssize_t store_cursor_blink(struct device *device,
> +static ssize_t cursor_blink_store(struct device *device,
>  				  struct device_attribute *attr,
>  				  const char *buf, size_t count)
>  {
> @@ -3281,32 +3281,30 @@ static ssize_t store_cursor_blink(struct device *device,
>  	return count;
>  }
>  
> -static struct device_attribute device_attrs[] = {
> -	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
> -	__ATTR(rotate_all, S_IWUSR, NULL, store_rotate_all),
> -	__ATTR(cursor_blink, S_IRUGO|S_IWUSR, show_cursor_blink,
> -	       store_cursor_blink),
> +static DEVICE_ATTR_RW(rotate);
> +static DEVICE_ATTR_WO(rotate_all);
> +static DEVICE_ATTR_RW(cursor_blink);
> +
> +static struct attribute *fbcon_device_attrs[] = {
> +	&dev_attr_rotate.attr,
> +	&dev_attr_rotate_all.attr,
> +	&dev_attr_cursor_blink.attr,
> +	NULL,

No trailing commas after sentinel values.

> +};
> +
> +static const struct attribute_group fbcon_device_attr_group = {
> +	.attrs		= fbcon_device_attrs,
>  };
>  
>  static int fbcon_init_device(void)
>  {
> -	int i, error = 0;
> +	int ret;
>  
>  	fbcon_has_sysfs = 1;
>  
> -	for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
> -		error = device_create_file(fbcon_device, &device_attrs[i]);
> -
> -		if (error)
> -			break;
> -	}
> -
> -	if (error) {
> -		while (--i >= 0)
> -			device_remove_file(fbcon_device, &device_attrs[i]);
> -
> +	ret = device_add_group(fbcon_device, &fbcon_device_attr_group);
> +	if (ret)
>  		fbcon_has_sysfs = 0;
> -	}
>  
>  	return 0;
>  }
> @@ -3389,11 +3387,9 @@ void __init fb_console_init(void)
>  
>  static void __exit fbcon_deinit_device(void)
>  {
> -	int i;
>  
>  	if (fbcon_has_sysfs) {
> -		for (i = 0; i < ARRAY_SIZE(device_attrs); i++)
> -			device_remove_file(fbcon_device, &device_attrs[i]);
> +		device_remove_group(fb_info->dev, &fbcon_device_attr_group);

Please at least compile-test your changes before submission.

>  
>  		fbcon_has_sysfs = 0;
>  	}

All of this can be simplified even more:

* fbcon_deinit_device() can be removed easily, as the attributes are
  automatically removed when the device is destroyed.
* Using device_create_with_groups() the device core will take complete care of
  the attribute lifecycle, also allowing to remove fbcon_init_device()


Thomas

Re: [PATCH 1/2] fbcon: Register sysfs groups through device_add_group

[Shixiong Ou]

在 2025/3/13 00:47, Thomas Weißschuh 写道:
> Hi,
>
> On Tue, Mar 11, 2025 at 07:28:55PM +0800, oushixiong1025@163.com wrote:
>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>
>> Use device_add_group() to simplify creation and removal.
>>
>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>> ---
>>   drivers/video/fbdev/core/fbcon.c | 48 +++++++++++++++-----------------
>>   1 file changed, 22 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>> index 07d127110ca4..51c3e8a5a092 100644
>> --- a/drivers/video/fbdev/core/fbcon.c
>> +++ b/drivers/video/fbdev/core/fbcon.c
>> @@ -3159,7 +3159,7 @@ static const struct consw fb_con = {
>>   	.con_debug_leave	= fbcon_debug_leave,
>>   };
>>   
>> -static ssize_t store_rotate(struct device *device,
>> +static ssize_t rotate_store(struct device *device,
>>   			    struct device_attribute *attr, const char *buf,
>>   			    size_t count)
>>   {
>> @@ -3181,7 +3181,7 @@ static ssize_t store_rotate(struct device *device,
>>   	return count;
>>   }
>>   
>> -static ssize_t store_rotate_all(struct device *device,
>> +static ssize_t rotate_all_store(struct device *device,
>>   				struct device_attribute *attr,const char *buf,
>>   				size_t count)
>>   {
>> @@ -3203,7 +3203,7 @@ static ssize_t store_rotate_all(struct device *device,
>>   	return count;
>>   }
>>   
>> -static ssize_t show_rotate(struct device *device,
>> +static ssize_t rotate_show(struct device *device,
>>   			   struct device_attribute *attr,char *buf)
>>   {
>>   	struct fb_info *info;
>> @@ -3222,7 +3222,7 @@ static ssize_t show_rotate(struct device *device,
>>   	return sysfs_emit(buf, "%d\n", rotate);
>>   }
>>   
>> -static ssize_t show_cursor_blink(struct device *device,
>> +static ssize_t cursor_blink_show(struct device *device,
>>   				 struct device_attribute *attr, char *buf)
>>   {
>>   	struct fb_info *info;
>> @@ -3247,7 +3247,7 @@ static ssize_t show_cursor_blink(struct device *device,
>>   	return sysfs_emit(buf, "%d\n", blink);
>>   }
>>   
>> -static ssize_t store_cursor_blink(struct device *device,
>> +static ssize_t cursor_blink_store(struct device *device,
>>   				  struct device_attribute *attr,
>>   				  const char *buf, size_t count)
>>   {
>> @@ -3281,32 +3281,30 @@ static ssize_t store_cursor_blink(struct device *device,
>>   	return count;
>>   }
>>   
>> -static struct device_attribute device_attrs[] = {
>> -	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
>> -	__ATTR(rotate_all, S_IWUSR, NULL, store_rotate_all),
>> -	__ATTR(cursor_blink, S_IRUGO|S_IWUSR, show_cursor_blink,
>> -	       store_cursor_blink),
>> +static DEVICE_ATTR_RW(rotate);
>> +static DEVICE_ATTR_WO(rotate_all);
>> +static DEVICE_ATTR_RW(cursor_blink);
>> +
>> +static struct attribute *fbcon_device_attrs[] = {
>> +	&dev_attr_rotate.attr,
>> +	&dev_attr_rotate_all.attr,
>> +	&dev_attr_cursor_blink.attr,
>> +	NULL,
> No trailing commas after sentinel values.
>
>> +};
>> +
>> +static const struct attribute_group fbcon_device_attr_group = {
>> +	.attrs		= fbcon_device_attrs,
>>   };
>>   
>>   static int fbcon_init_device(void)
>>   {
>> -	int i, error = 0;
>> +	int ret;
>>   
>>   	fbcon_has_sysfs = 1;
>>   
>> -	for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
>> -		error = device_create_file(fbcon_device, &device_attrs[i]);
>> -
>> -		if (error)
>> -			break;
>> -	}
>> -
>> -	if (error) {
>> -		while (--i >= 0)
>> -			device_remove_file(fbcon_device, &device_attrs[i]);
>> -
>> +	ret = device_add_group(fbcon_device, &fbcon_device_attr_group);
>> +	if (ret)
>>   		fbcon_has_sysfs = 0;
>> -	}
>>   
>>   	return 0;
>>   }
>> @@ -3389,11 +3387,9 @@ void __init fb_console_init(void)
>>   
>>   static void __exit fbcon_deinit_device(void)
>>   {
>> -	int i;
>>   
>>   	if (fbcon_has_sysfs) {
>> -		for (i = 0; i < ARRAY_SIZE(device_attrs); i++)
>> -			device_remove_file(fbcon_device, &device_attrs[i]);
>> +		device_remove_group(fb_info->dev, &fbcon_device_attr_group);
> Please at least compile-test your changes before submission.
>
>>   
>>   		fbcon_has_sysfs = 0;
>>   	}
> All of this can be simplified even more:
>
> * fbcon_deinit_device() can be removed easily, as the attributes are
>    automatically removed when the device is destroyed.
> * Using device_create_with_groups() the device core will take complete care of
>    the attribute lifecycle, also allowing to remove fbcon_init_device()

Thanks for your advice, I will fix and refactor this code in the next patch.

Thanks and Regards,
Shixiong Ou.

>
> Thomas