在 2024/10/28 17:44, Christoph Hellwig 写道:
On Mon, Oct 28, 2024 at 05:07:26PM +0800, Yang Erkun wrote:
Fix this problem by following what loop_init() does. Besides,
reintroduce brd_devices_mutex to help serialize modifications to
brd_list.
This looks generally good. Minor comments below:
Hi,
Thanks for your review!
+ snprintf(buf, DISK_NAME_LEN, "ram%d", i);
+ mutex_lock(&brd_devices_mutex);
+ list_for_each_entry(brd, &brd_devices, brd_list) {
+ if (brd->brd_number == i) {
+ mutex_unlock(&brd_devices_mutex);
+ err = -EEXIST;
+ goto out;
This now prints an error message for an already existing
device, which should not happen for the module_init case,
but will happen all the time for the probe callback, and
is really annoying. Please drop that part of the change.
OK, maybe print nothing is better like loop_add in loop_init has did to
leave code clean? mknod can help to create /dev/ram%d... Hi, Kuai, what
do you think?
+ }
+ }
brd = kzalloc(sizeof(*brd), GFP_KERNEL);
+ if (!brd) {
+ mutex_unlock(&brd_devices_mutex);
+ err = -ENOMEM;
+ goto out;
+ }
brd->brd_number = i;
list_add_tail(&brd->brd_list, &brd_devices);
+ mutex_unlock(&brd_devices_mutex);
And maybe just split the whole locked section into a
brd_find_or_alloc_device helper to make verifying the locking easier?
Ok.
+ mutex_lock(&brd_devices_mutex);
list_del(&brd->brd_list);
+ mutex_unlock(&brd_devices_mutex);
kfree(brd);
+ mutex_lock(&brd_devices_mutex);
list_del(&brd->brd_list);
+ mutex_unlock(&brd_devices_mutex);
kfree(brd);
Maybe a brd_free_device helper for this pattern would be nice as well.
Ok.
+ brd_check_and_reset_par();
+ brd_debugfs_dir = debugfs_create_dir("ramdisk_pages", NULL);
if (__register_blkdev(RAMDISK_MAJOR, "ramdisk", brd_probe)) {
- err = -EIO;
- goto out_free;
+ pr_info("brd: module NOT loaded !!!\n");
+ debugfs_remove_recursive(brd_debugfs_dir);
+ return -EIO;
I'd keep an error handling goto for this to keep the main path
straight.
Ok.
