Re: [PATCH 14/19] floppy: use a separate gendisk for each media format

From: Denis Efremov
Date: Fri Sep 04 2020 - 06:00:04 EST

Next message: Sylwester Nawrocki: "[PATCH] ARM: dts: samsung: odroid-xu3: Move assigned-clock* properties to i2s0 node"
Previous message: Qi Liu: "[PATCH] arm64: perf: Remove unnecessary event_idx check"
In reply to: Christoph Hellwig: "[PATCH 14/19] floppy: use a separate gendisk for each media format"
Next in thread: Hannes Reinecke: "Re: [PATCH 14/19] floppy: use a separate gendisk for each media format"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 9/3/20 11:01 AM, Christoph Hellwig wrote:
> The floppy driver usually autodetects the media when used with the
> normal /dev/fd? devices, which also are the only nodes created by udev.
> But it also supports various aliases that force a given media format.
> That is currently supported using the blk_register_region framework
> which finds the floppy gendisk even for a 'mismatched' dev_t. The
> problem with this (besides the code complexity) is that it creates
> multiple struct block_device instances for the whole device of a
> single gendisk, which can lead to interesting issues in code not
> aware of that fact.
>
> To fix this just create a separate gendisk for each of the aliases
> if they are accessed.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Tested-by: Denis Efremov <efremov@xxxxxxxxx>

The patch looks ok as it is. Two nitpicks below if you will send next revision.

> ---
> drivers/block/floppy.c | 154 ++++++++++++++++++++++++++---------------
> 1 file changed, 97 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index a563b023458a8b..f07d97558cb698 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -402,7 +402,6 @@ static struct floppy_drive_params drive_params[N_DRIVE];
> static struct floppy_drive_struct drive_state[N_DRIVE];
> static struct floppy_write_errors write_errors[N_DRIVE];
> static struct timer_list motor_off_timer[N_DRIVE];
> -static struct gendisk *disks[N_DRIVE];
> static struct blk_mq_tag_set tag_sets[N_DRIVE];
> static struct block_device *opened_bdev[N_DRIVE];
> static DEFINE_MUTEX(open_lock);
> @@ -477,6 +476,8 @@ static struct floppy_struct floppy_type[32] = {
> { 3200,20,2,80,0,0x1C,0x00,0xCF,0x2C,"H1600" }, /* 31 1.6MB 3.5" */
> };
>
> +static struct gendisk *disks[N_DRIVE][ARRAY_SIZE(floppy_type)];
> +
> #define SECTSIZE (_FD_SECTSIZE(*floppy))
>
> /* Auto-detection: Disk type used until the next media change occurs. */
> @@ -4109,7 +4110,7 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
>
> new_dev = MINOR(bdev->bd_dev);
> drive_state[drive].fd_device = new_dev;
> - set_capacity(disks[drive], floppy_sizes[new_dev]);
> + set_capacity(disks[drive][ITYPE(new_dev)], floppy_sizes[new_dev]);
> if (old_dev != -1 && old_dev != new_dev) {
> if (buffer_drive == drive)
> buffer_track = -1;
> @@ -4577,15 +4578,58 @@ static bool floppy_available(int drive)
> return true;
> }
>
> -static struct kobject *floppy_find(dev_t dev, int *part, void *data)
> +static int floppy_alloc_disk(unsigned int drive, unsigned int type)
> {
> - int drive = (*part & 3) | ((*part & 0x80) >> 5);
> - if (drive >= N_DRIVE || !floppy_available(drive))
> - return NULL;
> - if (((*part >> 2) & 0x1f) >= ARRAY_SIZE(floppy_type))
> - return NULL;
> - *part = 0;
> - return get_disk_and_module(disks[drive]);
> + struct gendisk *disk;
> + int err;
> +
> + disk = alloc_disk(1);
> + if (!disk)
> + return -ENOMEM;
> +
> + disk->queue = blk_mq_init_queue(&tag_sets[drive]);
> + if (IS_ERR(disk->queue)) {
> + err = PTR_ERR(disk->queue);
> + disk->queue = NULL;
> + put_disk(disk);
> + return err;
> + }
> +
> + blk_queue_bounce_limit(disk->queue, BLK_BOUNCE_HIGH);
> + blk_queue_max_hw_sectors(disk->queue, 64);
> + disk->major = FLOPPY_MAJOR;
> + disk->first_minor = TOMINOR(drive) | (type << 2);
> + disk->fops = &floppy_fops;
> + disk->events = DISK_EVENT_MEDIA_CHANGE;
> + if (type)
> + sprintf(disk->disk_name, "fd%d_type%d", drive, type);
> + else
> + sprintf(disk->disk_name, "fd%d", drive);
> + /* to be cleaned up... */
> + disk->private_data = (void *)(long)drive;
> + disk->flags |= GENHD_FL_REMOVABLE;
> +
> + disks[drive][type] = disk;
> + return 0;
> +}
> +
> +static DEFINE_MUTEX(floppy_probe_lock);
> +
> +static void floppy_probe(dev_t dev)
> +{
> + unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
> + unsigned int type = (MINOR(dev) >> 2) & 0x1f;

ITYPE(MINOR(dev))?

> +
> + if (drive >= N_DRIVE || !floppy_available(drive) ||
> + type >= ARRAY_SIZE(floppy_type))
> + return;
> +
> + mutex_lock(&floppy_probe_lock);
> + if (!disks[drive][type]) {
> + if (floppy_alloc_disk(drive, type) == 0)
> + add_disk(disks[drive][type]);
> + }
> + mutex_unlock(&floppy_probe_lock);
> }
>
> static int __init do_floppy_init(void)
> @@ -4607,33 +4651,25 @@ static int __init do_floppy_init(void)
> return -ENOMEM;
>
> for (drive = 0; drive < N_DRIVE; drive++) {
> - disks[drive] = alloc_disk(1);
> - if (!disks[drive]) {
> - err = -ENOMEM;
> + memset(&tag_sets[drive], 0, sizeof(tag_sets[drive]));
> + tag_sets[drive].ops = &floppy_mq_ops;
> + tag_sets[drive].nr_hw_queues = 1;
> + tag_sets[drive].nr_maps = 1;
> + tag_sets[drive].queue_depth = 2;
> + tag_sets[drive].numa_node = NUMA_NO_NODE;
> + tag_sets[drive].flags = BLK_MQ_F_SHOULD_MERGE;
> + err = blk_mq_alloc_tag_set(&tag_sets[drive]);
> + if (err)
> goto out_put_disk;
> - }
>
> - disks[drive]->queue = blk_mq_init_sq_queue(&tag_sets[drive],
> - &floppy_mq_ops, 2,
> - BLK_MQ_F_SHOULD_MERGE);
> - if (IS_ERR(disks[drive]->queue)) {
> - err = PTR_ERR(disks[drive]->queue);
> - disks[drive]->queue = NULL;
> + err = floppy_alloc_disk(drive, 0);
> + if (err)
> goto out_put_disk;
> - }
> -
> - blk_queue_bounce_limit(disks[drive]->queue, BLK_BOUNCE_HIGH);
> - blk_queue_max_hw_sectors(disks[drive]->queue, 64);
> - disks[drive]->major = FLOPPY_MAJOR;
> - disks[drive]->first_minor = TOMINOR(drive);
> - disks[drive]->fops = &floppy_fops;
> - disks[drive]->events = DISK_EVENT_MEDIA_CHANGE;
> - sprintf(disks[drive]->disk_name, "fd%d", drive);
>
> timer_setup(&motor_off_timer[drive], motor_off_callback, 0);
> }
>
> - err = register_blkdev(FLOPPY_MAJOR, "fd");
> + err = __register_blkdev(FLOPPY_MAJOR, "fd", floppy_probe);
> if (err)
> goto out_put_disk;
>
> @@ -4641,9 +4677,6 @@ static int __init do_floppy_init(void)
> if (err)
> goto out_unreg_blkdev;
>
> - blk_register_region(MKDEV(FLOPPY_MAJOR, 0), 256, THIS_MODULE,
> - floppy_find, NULL, NULL);
> -
> for (i = 0; i < 256; i++)
> if (ITYPE(i))
> floppy_sizes[i] = floppy_type[ITYPE(i)].size;
> @@ -4671,7 +4704,7 @@ static int __init do_floppy_init(void)
> if (fdc_state[0].address == -1) {
> cancel_delayed_work(&fd_timeout);
> err = -ENODEV;
> - goto out_unreg_region;
> + goto out_unreg_driver;
> }
> #if N_FDC > 1
> fdc_state[1].address = FDC2;
> @@ -4682,7 +4715,7 @@ static int __init do_floppy_init(void)
> if (err) {
> cancel_delayed_work(&fd_timeout);
> err = -EBUSY;
> - goto out_unreg_region;
> + goto out_unreg_driver;
> }
>
> /* initialise drive state */
> @@ -4759,10 +4792,8 @@ static int __init do_floppy_init(void)
> if (err)
> goto out_remove_drives;
>
> - /* to be cleaned up... */
> - disks[drive]->private_data = (void *)(long)drive;
> - disks[drive]->flags |= GENHD_FL_REMOVABLE;
> - device_add_disk(&floppy_device[drive].dev, disks[drive], NULL);
> + device_add_disk(&floppy_device[drive].dev, disks[drive][0],
> + NULL);
> }
>
> return 0;
> @@ -4770,30 +4801,27 @@ static int __init do_floppy_init(void)
> out_remove_drives:
> while (drive--) {
> if (floppy_available(drive)) {
> - del_gendisk(disks[drive]);
> + del_gendisk(disks[drive][0]);
> platform_device_unregister(&floppy_device[drive]);
> }
> }
> out_release_dma:
> if (atomic_read(&usage_count))
> floppy_release_irq_and_dma();
> -out_unreg_region:
> - blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
> +out_unreg_driver:
> platform_driver_unregister(&floppy_driver);
> out_unreg_blkdev:
> unregister_blkdev(FLOPPY_MAJOR, "fd");
> out_put_disk:
> destroy_workqueue(floppy_wq);
> for (drive = 0; drive < N_DRIVE; drive++) {
> - if (!disks[drive])
> + if (!disks[drive][0])
> break;
> - if (disks[drive]->queue) {
> - del_timer_sync(&motor_off_timer[drive]);
> - blk_cleanup_queue(disks[drive]->queue);
> - disks[drive]->queue = NULL;
> - blk_mq_free_tag_set(&tag_sets[drive]);
> - }
> - put_disk(disks[drive]);
> + del_timer_sync(&motor_off_timer[drive]);
> + blk_cleanup_queue(disks[drive][0]->queue);
> + disks[drive][0]->queue = NULL;
> + blk_mq_free_tag_set(&tag_sets[drive]);
> + put_disk(disks[drive][0]);
> }
> return err;
> }
> @@ -5004,9 +5032,8 @@ module_init(floppy_module_init);
>
> static void __exit floppy_module_exit(void)
> {
> - int drive;
> + int drive, i;
>
> - blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
> unregister_blkdev(FLOPPY_MAJOR, "fd");
> platform_driver_unregister(&floppy_driver);
>
> @@ -5016,10 +5043,16 @@ static void __exit floppy_module_exit(void)
> del_timer_sync(&motor_off_timer[drive]);
>
> if (floppy_available(drive)) {
> - del_gendisk(disks[drive]);
> + for (i = 0; i < ARRAY_SIZE(floppy_type); i++) {
> + if (disks[drive][i])
> + del_gendisk(disks[drive][i]);
> + }> platform_device_unregister(&floppy_device[drive]);
> }
> - blk_cleanup_queue(disks[drive]->queue);
> + for (i = 0; i < ARRAY_SIZE(floppy_type); i++) {
> + if (disks[drive][i])
> + blk_cleanup_queue(disks[drive][i]->queue);
> + }
> blk_mq_free_tag_set(&tag_sets[drive]);
>
> /*
> @@ -5027,10 +5060,17 @@ static void __exit floppy_module_exit(void)
> * queue reference in put_disk().
> */
> if (!(allowed_drive_mask & (1 << drive)) ||
> - fdc_state[FDC(drive)].version == FDC_NONE)
> - disks[drive]->queue = NULL;
> + fdc_state[FDC(drive)].version == FDC_NONE) {
> + for (i = 0; i < ARRAY_SIZE(floppy_type); i++) {
> + if (disks[drive][i])
> + disks[drive][i]->queue = NULL;
> + }
> + }
>
> - put_disk(disks[drive]);
> + for (i = 0; i < ARRAY_SIZE(floppy_type); i++) {
> + if (disks[drive][i])

We can omit NULL check for put_disk().

> + put_disk(disks[drive][i]);
> + }

Regards,
Denis

Next message: Sylwester Nawrocki: "[PATCH] ARM: dts: samsung: odroid-xu3: Move assigned-clock* properties to i2s0 node"
Previous message: Qi Liu: "[PATCH] arm64: perf: Remove unnecessary event_idx check"
In reply to: Christoph Hellwig: "[PATCH 14/19] floppy: use a separate gendisk for each media format"
Next in thread: Hannes Reinecke: "Re: [PATCH 14/19] floppy: use a separate gendisk for each media format"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]