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:
> + 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.
> + }
> + }
> 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?
> + 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.
> + 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.
