Re: [PATCH 11/15] zram: Pass attribute group to device_add_disk

[Sergey Senozhatsky]

On (08/18/16 10:59), Sergey Senozhatsky wrote:
[..]
> I like the previous "Error creating sysfs group for device" string better,
> than "Error creating disk", because the latter one is much less informative.
> 
> do you want to do something like below?
> 
> int device_add_disk(struct device *parent, struct gendisk *disk,
> ...
>if (attr_group) {
>retval = sysfs_create_group(_to_dev(disk)->kobj,
>attr_group);
> 
> + pr_err("Error creating sysfs group for device ...\n", ...);

d'oh... a typo. pr_err() is meant to be in `if (retval)' branch.

>if (retval)
>goto fail;
>}

-ss

Re: [PATCH 11/15] zram: Pass attribute group to device_add_disk

[Sergey Senozhatsky]

Hello,

On (08/17/16 15:15), Fam Zheng wrote:
[..]
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 20920a2..2331788 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1298,13 +1298,10 @@ static int zram_add(void)
>   zram->disk->queue->limits.discard_zeroes_data = 0;
>   queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, zram->disk->queue);
>  
> - device_add_disk(NULL, zram->disk, NULL);
> + ret = device_add_disk(NULL, zram->disk, _disk_attr_group);
>  
> - ret = sysfs_create_group(_to_dev(zram->disk)->kobj,
> - _disk_attr_group);
>   if (ret < 0) {
> - pr_err("Error creating sysfs group for device %d\n",
> - device_id);
> + pr_err("Error creating disk %d\n", device_id);
>   goto out_free_disk;
>   }
>   strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));

I like the previous "Error creating sysfs group for device" string better,
than "Error creating disk", because the latter one is much less informative.

do you want to do something like below?

int device_add_disk(struct device *parent, struct gendisk *disk,
...
   if (attr_group) {
   retval = sysfs_create_group(_to_dev(disk)->kobj,
   attr_group);

+   pr_err("Error creating sysfs group for device ...\n", ...);

   if (retval)
   goto fail;
   }

-ss

[PATCH 11/15] zram: Pass attribute group to device_add_disk

[Fam Zheng]

Previously after device_add_disk returns, the KOBJ_ADD uevent is already
emitted. Adding attributes after that is a poor usage of kobject, and
in practice may result in race conditions with userspace, for
example udev checks availability of certain attributes and initializes
/dev entries conditionally.

device_add_disk can handle adding attribute group better, so use it.
Meanwhile, update the error check code and message.

Signed-off-by: Fam Zheng 
---
 drivers/block/zram/zram_drv.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 20920a2..2331788 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1298,13 +1298,10 @@ static int zram_add(void)
zram->disk->queue->limits.discard_zeroes_data = 0;
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, zram->disk->queue);
 
-   device_add_disk(NULL, zram->disk, NULL);
+   ret = device_add_disk(NULL, zram->disk, _disk_attr_group);
 
-   ret = sysfs_create_group(_to_dev(zram->disk)->kobj,
-   _disk_attr_group);
if (ret < 0) {
-   pr_err("Error creating sysfs group for device %d\n",
-   device_id);
+   pr_err("Error creating disk %d\n", device_id);
goto out_free_disk;
}
strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
-- 
2.7.4