2020-04-23 07:17:08

by Christoph Hellwig

[permalink] [raw]
Subject: stop using ioctl_by_bdev for file system access to CDROMs

Hi Jens,

except for the DASD case under discussion the last users of ioctl_by_bdev
are the file system drivers that want to query CDROM information using
ioctls. This series switches them to use function calls directly into
the CDROM midlayer instead, which implies:

- adding a cdrom_device_info pointer to the gendisk, so that file systems
can find it without going to the low-level driver first
- ensuring that the CDROM midlayer (which isn't a lot of code) is built
in if the file systems are built in so that they can actually call the
exported functions


2020-04-23 07:17:09

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 1/7] block: add a cdrom_device_info pointer to struct gendisk

Add a pointer to the CDROM information structure to struct gendisk.
This will allow various removable media file systems to call directly
into the CDROM layer instead of abusing ioctls with kernel pointers.

Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/block/paride/pcd.c | 2 +-
drivers/cdrom/cdrom.c | 5 ++++-
drivers/cdrom/gdrom.c | 2 +-
drivers/ide/ide-cd.c | 3 +--
drivers/scsi/sr.c | 3 +--
include/linux/cdrom.h | 2 +-
include/linux/genhd.h | 9 +++++++++
7 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index cda5cf917e9a..5124eca90e83 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -1032,7 +1032,7 @@ static int __init pcd_init(void)

for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++) {
if (cd->present) {
- register_cdrom(&cd->info);
+ register_cdrom(cd->disk, &cd->info);
cd->disk->private_data = cd;
add_disk(cd->disk);
}
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index faca0f346fff..a1d2112fd283 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -586,7 +586,7 @@ static int cdrom_mrw_set_lba_space(struct cdrom_device_info *cdi, int space)
return 0;
}

-int register_cdrom(struct cdrom_device_info *cdi)
+int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
{
static char banner_printed;
const struct cdrom_device_ops *cdo = cdi->ops;
@@ -601,6 +601,9 @@ int register_cdrom(struct cdrom_device_info *cdi)
cdrom_sysctl_register();
}

+ cdi->disk = disk;
+ disk->cdi = cdi;
+
ENSURE(cdo, drive_status, CDC_DRIVE_STATUS);
if (cdo->check_events == NULL && cdo->media_changed == NULL)
WARN_ON_ONCE(cdo->capability & (CDC_MEDIA_CHANGED | CDC_SELECT_DISC));
diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index c51292c2a131..09b0cd292720 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -770,7 +770,7 @@ static int probe_gdrom(struct platform_device *devptr)
goto probe_fail_no_disk;
}
probe_gdrom_setupdisk();
- if (register_cdrom(gd.cd_info)) {
+ if (register_cdrom(gd.disk, gd.cd_info)) {
err = -ENODEV;
goto probe_fail_cdrom_register;
}
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index dcf8b51b47fd..40e124eb918a 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1305,8 +1305,7 @@ static int ide_cdrom_register(ide_drive_t *drive, int nslots)
if (drive->atapi_flags & IDE_AFLAG_NO_SPEED_SELECT)
devinfo->mask |= CDC_SELECT_SPEED;

- devinfo->disk = info->disk;
- return register_cdrom(devinfo);
+ return register_cdrom(info->disk, devinfo);
}

static int ide_cdrom_probe_capabilities(ide_drive_t *drive)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index d2fe3fa470f9..f9b589d60a46 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -794,9 +794,8 @@ static int sr_probe(struct device *dev)
set_capacity(disk, cd->capacity);
disk->private_data = &cd->driver;
disk->queue = sdev->request_queue;
- cd->cdi.disk = disk;

- if (register_cdrom(&cd->cdi))
+ if (register_cdrom(disk, &cd->cdi))
goto fail_put;

/*
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 528271c60018..4f74ce050253 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -104,7 +104,7 @@ extern unsigned int cdrom_check_events(struct cdrom_device_info *cdi,
unsigned int clearing);
extern int cdrom_media_changed(struct cdrom_device_info *);

-extern int register_cdrom(struct cdrom_device_info *cdi);
+extern int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi);
extern void unregister_cdrom(struct cdrom_device_info *cdi);

typedef struct {
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 058d895544c7..f9c226f9546a 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -217,11 +217,20 @@ struct gendisk {
#ifdef CONFIG_BLK_DEV_INTEGRITY
struct kobject integrity_kobj;
#endif /* CONFIG_BLK_DEV_INTEGRITY */
+#if IS_ENABLED(CONFIG_CDROM)
+ struct cdrom_device_info *cdi;
+#endif
int node_id;
struct badblocks *bb;
struct lockdep_map lockdep_map;
};

+#if IS_REACHABLE(CONFIG_CDROM)
+#define disk_to_cdi(disk) ((disk)->cdi)
+#else
+#define disk_to_cdi(disk) NULL
+#endif
+
static inline struct gendisk *part_to_disk(struct hd_struct *part)
{
if (likely(part)) {
--
2.26.1

2020-04-23 07:17:14

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 3/7] cdrom: factor out a cdrom_read_tocentry helper

Factor out a version of the CDROMREADTOCENTRY ioctl handler that can
be called directly from kernel space.

Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/cdrom/cdrom.c | 39 ++++++++++++++++++++++-----------------
include/linux/cdrom.h | 3 +++
2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index a1d2112fd283..c91d1e138214 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2666,32 +2666,37 @@ static int cdrom_ioctl_read_tochdr(struct cdrom_device_info *cdi,
return 0;
}

+int cdrom_read_tocentry(struct cdrom_device_info *cdi,
+ struct cdrom_tocentry *entry)
+{
+ u8 requested_format = entry->cdte_format;
+ int ret;
+
+ if (requested_format != CDROM_MSF && requested_format != CDROM_LBA)
+ return -EINVAL;
+
+ /* make interface to low-level uniform */
+ entry->cdte_format = CDROM_MSF;
+ ret = cdi->ops->audio_ioctl(cdi, CDROMREADTOCENTRY, entry);
+ if (!ret)
+ sanitize_format(&entry->cdte_addr, &entry->cdte_format,
+ requested_format);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(cdrom_read_tocentry);
+
static int cdrom_ioctl_read_tocentry(struct cdrom_device_info *cdi,
void __user *argp)
{
struct cdrom_tocentry entry;
- u8 requested_format;
int ret;

- /* cd_dbg(CD_DO_IOCTL, "entering CDROMREADTOCENTRY\n"); */
-
if (copy_from_user(&entry, argp, sizeof(entry)))
return -EFAULT;
-
- requested_format = entry.cdte_format;
- if (requested_format != CDROM_MSF && requested_format != CDROM_LBA)
- return -EINVAL;
- /* make interface to low-level uniform */
- entry.cdte_format = CDROM_MSF;
- ret = cdi->ops->audio_ioctl(cdi, CDROMREADTOCENTRY, &entry);
- if (ret)
- return ret;
- sanitize_format(&entry.cdte_addr, &entry.cdte_format, requested_format);
-
- if (copy_to_user(argp, &entry, sizeof(entry)))
+ ret = cdrom_read_tocentry(cdi, &entry);
+ if (!ret && copy_to_user(argp, &entry, sizeof(entry)))
return -EFAULT;
- /* cd_dbg(CD_DO_IOCTL, "CDROMREADTOCENTRY successful\n"); */
- return 0;
+ return ret;
}

static int cdrom_ioctl_play_msf(struct cdrom_device_info *cdi,
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 4f74ce050253..008c4d79fa33 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -94,6 +94,9 @@ struct cdrom_device_ops {
struct packet_command *);
};

+int cdrom_read_tocentry(struct cdrom_device_info *cdi,
+ struct cdrom_tocentry *entry);
+
/* the general block_device operations structure: */
extern int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
fmode_t mode);
--
2.26.1

2020-04-23 07:17:58

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 6/7] isofs: stop using ioctl_by_bdev

Instead just call the CD-ROM layer functionality directly, and turn the
hot mess in isofs_get_last_session into remotely readable code.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/isofs/inode.c | 54 +++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 62c0462dc89f..fc48923a9b6c 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -544,43 +544,41 @@ static int isofs_show_options(struct seq_file *m, struct dentry *root)

static unsigned int isofs_get_last_session(struct super_block *sb, s32 session)
{
- struct cdrom_multisession ms_info;
- unsigned int vol_desc_start;
- struct block_device *bdev = sb->s_bdev;
- int i;
+ struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
+ unsigned int vol_desc_start = 0;

- vol_desc_start=0;
- ms_info.addr_format=CDROM_LBA;
if (session > 0) {
- struct cdrom_tocentry Te;
- Te.cdte_track=session;
- Te.cdte_format=CDROM_LBA;
- i = ioctl_by_bdev(bdev, CDROMREADTOCENTRY, (unsigned long) &Te);
- if (!i) {
+ struct cdrom_tocentry te;
+
+ if (!cdi)
+ return -EINVAL;
+
+ te.cdte_track = session;
+ te.cdte_format = CDROM_LBA;
+ if (cdrom_read_tocentry(cdi, &te) == 0) {
printk(KERN_DEBUG "ISOFS: Session %d start %d type %d\n",
- session, Te.cdte_addr.lba,
- Te.cdte_ctrl&CDROM_DATA_TRACK);
- if ((Te.cdte_ctrl&CDROM_DATA_TRACK) == 4)
- return Te.cdte_addr.lba;
+ session, te.cdte_addr.lba,
+ te.cdte_ctrl & CDROM_DATA_TRACK);
+ if ((te.cdte_ctrl & CDROM_DATA_TRACK) == 4)
+ return te.cdte_addr.lba;
}

printk(KERN_ERR "ISOFS: Invalid session number or type of track\n");
}
- i = ioctl_by_bdev(bdev, CDROMMULTISESSION, (unsigned long) &ms_info);
- if (session > 0)
- printk(KERN_ERR "ISOFS: Invalid session number\n");
-#if 0
- printk(KERN_DEBUG "isofs.inode: CDROMMULTISESSION: rc=%d\n",i);
- if (i==0) {
- printk(KERN_DEBUG "isofs.inode: XA disk: %s\n",ms_info.xa_flag?"yes":"no");
- printk(KERN_DEBUG "isofs.inode: vol_desc_start = %d\n", ms_info.addr.lba);
- }
-#endif
- if (i==0)
+
+ if (cdi) {
+ struct cdrom_multisession ms_info;
+
+ ms_info.addr_format = CDROM_LBA;
+ if (cdrom_multisession(cdi, &ms_info) == 0) {
#if WE_OBEY_THE_WRITTEN_STANDARDS
- if (ms_info.xa_flag) /* necessary for a valid ms_info.addr */
+ /* necessary for a valid ms_info.addr */
+ if (ms_info.xa_flag)
#endif
- vol_desc_start=ms_info.addr.lba;
+ vol_desc_start = ms_info.addr.lba;
+ }
+ }
+
return vol_desc_start;
}

--
2.26.1

2020-04-23 07:18:44

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 4/7] cdrom: factor out a cdrom_multisession helper

Factor out a version of the CDROMMULTISESSION: ioctl handler that can
be called directly from kernel space.

Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/cdrom/cdrom.c | 41 +++++++++++++++++++++++++----------------
include/linux/cdrom.h | 2 ++
2 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index c91d1e138214..06896c07b133 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2295,37 +2295,46 @@ static int cdrom_read_cdda(struct cdrom_device_info *cdi, __u8 __user *ubuf,
return cdrom_read_cdda_old(cdi, ubuf, lba, nframes);
}

-static int cdrom_ioctl_multisession(struct cdrom_device_info *cdi,
- void __user *argp)
+int cdrom_multisession(struct cdrom_device_info *cdi,
+ struct cdrom_multisession *info)
{
- struct cdrom_multisession ms_info;
u8 requested_format;
int ret;

- cd_dbg(CD_DO_IOCTL, "entering CDROMMULTISESSION\n");
-
if (!(cdi->ops->capability & CDC_MULTI_SESSION))
return -ENOSYS;

- if (copy_from_user(&ms_info, argp, sizeof(ms_info)))
- return -EFAULT;
-
- requested_format = ms_info.addr_format;
+ requested_format = info->addr_format;
if (requested_format != CDROM_MSF && requested_format != CDROM_LBA)
return -EINVAL;
- ms_info.addr_format = CDROM_LBA;
+ info->addr_format = CDROM_LBA;

- ret = cdi->ops->get_last_session(cdi, &ms_info);
- if (ret)
- return ret;
+ ret = cdi->ops->get_last_session(cdi, info);
+ if (!ret)
+ sanitize_format(&info->addr, &info->addr_format,
+ requested_format);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(cdrom_multisession);

- sanitize_format(&ms_info.addr, &ms_info.addr_format, requested_format);
+static int cdrom_ioctl_multisession(struct cdrom_device_info *cdi,
+ void __user *argp)
+{
+ struct cdrom_multisession info;
+ int ret;
+
+ cd_dbg(CD_DO_IOCTL, "entering CDROMMULTISESSION\n");

- if (copy_to_user(argp, &ms_info, sizeof(ms_info)))
+ if (copy_from_user(&info, argp, sizeof(info)))
+ return -EFAULT;
+ ret = cdrom_multisession(cdi, &info);
+ if (ret)
+ return ret;
+ if (copy_to_user(argp, &info, sizeof(info)))
return -EFAULT;

cd_dbg(CD_DO_IOCTL, "CDROMMULTISESSION successful\n");
- return 0;
+ return ret;
}

static int cdrom_ioctl_eject(struct cdrom_device_info *cdi)
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 008c4d79fa33..8543fa59da72 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -94,6 +94,8 @@ struct cdrom_device_ops {
struct packet_command *);
};

+int cdrom_multisession(struct cdrom_device_info *cdi,
+ struct cdrom_multisession *info);
int cdrom_read_tocentry(struct cdrom_device_info *cdi,
struct cdrom_tocentry *entry);

--
2.26.1

2020-04-23 07:18:48

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 7/7] udf: stop using ioctl_by_bdev

Instead just call the CD-ROM layer functionality directly.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/udf/lowlevel.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/fs/udf/lowlevel.c b/fs/udf/lowlevel.c
index 5c7ec121990d..f1094cdcd6cd 100644
--- a/fs/udf/lowlevel.c
+++ b/fs/udf/lowlevel.c
@@ -27,41 +27,38 @@

unsigned int udf_get_last_session(struct super_block *sb)
{
+ struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
struct cdrom_multisession ms_info;
- unsigned int vol_desc_start;
- struct block_device *bdev = sb->s_bdev;
- int i;

- vol_desc_start = 0;
- ms_info.addr_format = CDROM_LBA;
- i = ioctl_by_bdev(bdev, CDROMMULTISESSION, (unsigned long)&ms_info);
+ if (!cdi) {
+ udf_debug("CDROMMULTISESSION not supported.\n");
+ return 0;
+ }

- if (i == 0) {
+ ms_info.addr_format = CDROM_LBA;
+ if (cdrom_multisession(cdi, &ms_info) == 0) {
udf_debug("XA disk: %s, vol_desc_start=%d\n",
ms_info.xa_flag ? "yes" : "no", ms_info.addr.lba);
if (ms_info.xa_flag) /* necessary for a valid ms_info.addr */
- vol_desc_start = ms_info.addr.lba;
- } else {
- udf_debug("CDROMMULTISESSION not supported: rc=%d\n", i);
+ return ms_info.addr.lba;
}
- return vol_desc_start;
+ return 0;
}

unsigned long udf_get_last_block(struct super_block *sb)
{
struct block_device *bdev = sb->s_bdev;
+ struct cdrom_device_info *cdi = disk_to_cdi(bdev->bd_disk);
unsigned long lblock = 0;

/*
- * ioctl failed or returned obviously bogus value?
+ * The cdrom layer call failed or returned obviously bogus value?
* Try using the device size...
*/
- if (ioctl_by_bdev(bdev, CDROM_LAST_WRITTEN, (unsigned long) &lblock) ||
- lblock == 0)
+ if (!cdi || cdrom_get_last_written(cdi, &lblock) || lblock == 0)
lblock = i_size_read(bdev->bd_inode) >> sb->s_blocksize_bits;

if (lblock)
return lblock - 1;
- else
- return 0;
+ return 0;
}
--
2.26.1

2020-04-23 07:19:02

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 5/7] hfsplus: stop using ioctl_by_bdev

Instead just call the CD-ROM layer functionality directly.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/hfsplus/wrapper.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
index 08c1580bdf7a..61eec628805d 100644
--- a/fs/hfsplus/wrapper.c
+++ b/fs/hfsplus/wrapper.c
@@ -127,31 +127,34 @@ static int hfsplus_read_mdb(void *bufptr, struct hfsplus_wd *wd)
static int hfsplus_get_last_session(struct super_block *sb,
sector_t *start, sector_t *size)
{
- struct cdrom_multisession ms_info;
- struct cdrom_tocentry te;
- int res;
+ struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);

/* default values */
*start = 0;
*size = i_size_read(sb->s_bdev->bd_inode) >> 9;

if (HFSPLUS_SB(sb)->session >= 0) {
+ struct cdrom_tocentry te;
+
+ if (!cdi)
+ return -EINVAL;
+
te.cdte_track = HFSPLUS_SB(sb)->session;
te.cdte_format = CDROM_LBA;
- res = ioctl_by_bdev(sb->s_bdev,
- CDROMREADTOCENTRY, (unsigned long)&te);
- if (!res && (te.cdte_ctrl & CDROM_DATA_TRACK) == 4) {
- *start = (sector_t)te.cdte_addr.lba << 2;
- return 0;
+ if (cdrom_read_tocentry(cdi, &te) ||
+ (te.cdte_ctrl & CDROM_DATA_TRACK) != 4) {
+ pr_err("invalid session number or type of track\n");
+ return -EINVAL;
}
- pr_err("invalid session number or type of track\n");
- return -EINVAL;
+ *start = (sector_t)te.cdte_addr.lba << 2;
+ } else if (cdi) {
+ struct cdrom_multisession ms_info;
+
+ ms_info.addr_format = CDROM_LBA;
+ if (cdrom_multisession(cdi, &ms_info) == 0 && ms_info.xa_flag)
+ *start = (sector_t)ms_info.addr.lba << 2;
}
- ms_info.addr_format = CDROM_LBA;
- res = ioctl_by_bdev(sb->s_bdev, CDROMMULTISESSION,
- (unsigned long)&ms_info);
- if (!res && ms_info.xa_flag)
- *start = (sector_t)ms_info.addr.lba << 2;
+
return 0;
}

--
2.26.1

2020-04-23 07:42:42

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 1/7] block: add a cdrom_device_info pointer to struct gendisk

On 2020/04/23 16:15, Christoph Hellwig wrote:
> Add a pointer to the CDROM information structure to struct gendisk.
> This will allow various removable media file systems to call directly
> into the CDROM layer instead of abusing ioctls with kernel pointers.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> drivers/block/paride/pcd.c | 2 +-
> drivers/cdrom/cdrom.c | 5 ++++-
> drivers/cdrom/gdrom.c | 2 +-
> drivers/ide/ide-cd.c | 3 +--
> drivers/scsi/sr.c | 3 +--
> include/linux/cdrom.h | 2 +-
> include/linux/genhd.h | 9 +++++++++
> 7 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
> index cda5cf917e9a..5124eca90e83 100644
> --- a/drivers/block/paride/pcd.c
> +++ b/drivers/block/paride/pcd.c
> @@ -1032,7 +1032,7 @@ static int __init pcd_init(void)
>
> for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++) {
> if (cd->present) {
> - register_cdrom(&cd->info);
> + register_cdrom(cd->disk, &cd->info);
> cd->disk->private_data = cd;
> add_disk(cd->disk);
> }
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index faca0f346fff..a1d2112fd283 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -586,7 +586,7 @@ static int cdrom_mrw_set_lba_space(struct cdrom_device_info *cdi, int space)
> return 0;
> }
>
> -int register_cdrom(struct cdrom_device_info *cdi)
> +int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
> {
> static char banner_printed;
> const struct cdrom_device_ops *cdo = cdi->ops;
> @@ -601,6 +601,9 @@ int register_cdrom(struct cdrom_device_info *cdi)
> cdrom_sysctl_register();
> }
>
> + cdi->disk = disk;
> + disk->cdi = cdi;
> +
> ENSURE(cdo, drive_status, CDC_DRIVE_STATUS);
> if (cdo->check_events == NULL && cdo->media_changed == NULL)
> WARN_ON_ONCE(cdo->capability & (CDC_MEDIA_CHANGED | CDC_SELECT_DISC));
> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> index c51292c2a131..09b0cd292720 100644
> --- a/drivers/cdrom/gdrom.c
> +++ b/drivers/cdrom/gdrom.c
> @@ -770,7 +770,7 @@ static int probe_gdrom(struct platform_device *devptr)
> goto probe_fail_no_disk;
> }
> probe_gdrom_setupdisk();
> - if (register_cdrom(gd.cd_info)) {
> + if (register_cdrom(gd.disk, gd.cd_info)) {
> err = -ENODEV;
> goto probe_fail_cdrom_register;
> }
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index dcf8b51b47fd..40e124eb918a 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1305,8 +1305,7 @@ static int ide_cdrom_register(ide_drive_t *drive, int nslots)
> if (drive->atapi_flags & IDE_AFLAG_NO_SPEED_SELECT)
> devinfo->mask |= CDC_SELECT_SPEED;
>
> - devinfo->disk = info->disk;
> - return register_cdrom(devinfo);
> + return register_cdrom(info->disk, devinfo);
> }
>
> static int ide_cdrom_probe_capabilities(ide_drive_t *drive)
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index d2fe3fa470f9..f9b589d60a46 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -794,9 +794,8 @@ static int sr_probe(struct device *dev)
> set_capacity(disk, cd->capacity);
> disk->private_data = &cd->driver;
> disk->queue = sdev->request_queue;
> - cd->cdi.disk = disk;
>
> - if (register_cdrom(&cd->cdi))
> + if (register_cdrom(disk, &cd->cdi))
> goto fail_put;
>
> /*
> diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
> index 528271c60018..4f74ce050253 100644
> --- a/include/linux/cdrom.h
> +++ b/include/linux/cdrom.h
> @@ -104,7 +104,7 @@ extern unsigned int cdrom_check_events(struct cdrom_device_info *cdi,
> unsigned int clearing);
> extern int cdrom_media_changed(struct cdrom_device_info *);
>
> -extern int register_cdrom(struct cdrom_device_info *cdi);
> +extern int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi);
> extern void unregister_cdrom(struct cdrom_device_info *cdi);
>
> typedef struct {
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 058d895544c7..f9c226f9546a 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -217,11 +217,20 @@ struct gendisk {
> #ifdef CONFIG_BLK_DEV_INTEGRITY
> struct kobject integrity_kobj;
> #endif /* CONFIG_BLK_DEV_INTEGRITY */
> +#if IS_ENABLED(CONFIG_CDROM)
> + struct cdrom_device_info *cdi;
> +#endif
> int node_id;
> struct badblocks *bb;
> struct lockdep_map lockdep_map;
> };
>
> +#if IS_REACHABLE(CONFIG_CDROM)
> +#define disk_to_cdi(disk) ((disk)->cdi)
> +#else
> +#define disk_to_cdi(disk) NULL
> +#endif
> +
> static inline struct gendisk *part_to_disk(struct hd_struct *part)
> {
> if (likely(part)) {
>

Looks OK to me.

Reviewed-by: Damien Le Moal <[email protected]>

--
Damien Le Moal
Western Digital Research

2020-04-23 07:43:37

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 3/7] cdrom: factor out a cdrom_read_tocentry helper

On 2020/04/23 16:15, Christoph Hellwig wrote:
> Factor out a version of the CDROMREADTOCENTRY ioctl handler that can
> be called directly from kernel space.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> drivers/cdrom/cdrom.c | 39 ++++++++++++++++++++++-----------------
> include/linux/cdrom.h | 3 +++
> 2 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index a1d2112fd283..c91d1e138214 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -2666,32 +2666,37 @@ static int cdrom_ioctl_read_tochdr(struct cdrom_device_info *cdi,
> return 0;
> }
>
> +int cdrom_read_tocentry(struct cdrom_device_info *cdi,
> + struct cdrom_tocentry *entry)
> +{
> + u8 requested_format = entry->cdte_format;
> + int ret;
> +
> + if (requested_format != CDROM_MSF && requested_format != CDROM_LBA)
> + return -EINVAL;
> +
> + /* make interface to low-level uniform */
> + entry->cdte_format = CDROM_MSF;
> + ret = cdi->ops->audio_ioctl(cdi, CDROMREADTOCENTRY, entry);
> + if (!ret)
> + sanitize_format(&entry->cdte_addr, &entry->cdte_format,
> + requested_format);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdrom_read_tocentry);
> +
> static int cdrom_ioctl_read_tocentry(struct cdrom_device_info *cdi,
> void __user *argp)
> {
> struct cdrom_tocentry entry;
> - u8 requested_format;
> int ret;
>
> - /* cd_dbg(CD_DO_IOCTL, "entering CDROMREADTOCENTRY\n"); */
> -
> if (copy_from_user(&entry, argp, sizeof(entry)))
> return -EFAULT;
> -
> - requested_format = entry.cdte_format;
> - if (requested_format != CDROM_MSF && requested_format != CDROM_LBA)
> - return -EINVAL;
> - /* make interface to low-level uniform */
> - entry.cdte_format = CDROM_MSF;
> - ret = cdi->ops->audio_ioctl(cdi, CDROMREADTOCENTRY, &entry);
> - if (ret)
> - return ret;
> - sanitize_format(&entry.cdte_addr, &entry.cdte_format, requested_format);
> -
> - if (copy_to_user(argp, &entry, sizeof(entry)))
> + ret = cdrom_read_tocentry(cdi, &entry);
> + if (!ret && copy_to_user(argp, &entry, sizeof(entry)))
> return -EFAULT;
> - /* cd_dbg(CD_DO_IOCTL, "CDROMREADTOCENTRY successful\n"); */
> - return 0;
> + return ret;
> }
>
> static int cdrom_ioctl_play_msf(struct cdrom_device_info *cdi,
> diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
> index 4f74ce050253..008c4d79fa33 100644
> --- a/include/linux/cdrom.h
> +++ b/include/linux/cdrom.h
> @@ -94,6 +94,9 @@ struct cdrom_device_ops {
> struct packet_command *);
> };
>
> +int cdrom_read_tocentry(struct cdrom_device_info *cdi,
> + struct cdrom_tocentry *entry);
> +
> /* the general block_device operations structure: */
> extern int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
> fmode_t mode);
>

Looks OK to me.

Reviewed-by: Damien Le Moal <[email protected]>


--
Damien Le Moal
Western Digital Research

2020-04-23 07:44:15

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 4/7] cdrom: factor out a cdrom_multisession helper

On 2020/04/23 16:15, Christoph Hellwig wrote:
> Factor out a version of the CDROMMULTISESSION: ioctl handler that can
> be called directly from kernel space.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> drivers/cdrom/cdrom.c | 41 +++++++++++++++++++++++++----------------
> include/linux/cdrom.h | 2 ++
> 2 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index c91d1e138214..06896c07b133 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -2295,37 +2295,46 @@ static int cdrom_read_cdda(struct cdrom_device_info *cdi, __u8 __user *ubuf,
> return cdrom_read_cdda_old(cdi, ubuf, lba, nframes);
> }
>
> -static int cdrom_ioctl_multisession(struct cdrom_device_info *cdi,
> - void __user *argp)
> +int cdrom_multisession(struct cdrom_device_info *cdi,
> + struct cdrom_multisession *info)
> {
> - struct cdrom_multisession ms_info;
> u8 requested_format;
> int ret;
>
> - cd_dbg(CD_DO_IOCTL, "entering CDROMMULTISESSION\n");
> -
> if (!(cdi->ops->capability & CDC_MULTI_SESSION))
> return -ENOSYS;
>
> - if (copy_from_user(&ms_info, argp, sizeof(ms_info)))
> - return -EFAULT;
> -
> - requested_format = ms_info.addr_format;
> + requested_format = info->addr_format;
> if (requested_format != CDROM_MSF && requested_format != CDROM_LBA)
> return -EINVAL;
> - ms_info.addr_format = CDROM_LBA;
> + info->addr_format = CDROM_LBA;
>
> - ret = cdi->ops->get_last_session(cdi, &ms_info);
> - if (ret)
> - return ret;
> + ret = cdi->ops->get_last_session(cdi, info);
> + if (!ret)
> + sanitize_format(&info->addr, &info->addr_format,
> + requested_format);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdrom_multisession);
>
> - sanitize_format(&ms_info.addr, &ms_info.addr_format, requested_format);
> +static int cdrom_ioctl_multisession(struct cdrom_device_info *cdi,
> + void __user *argp)
> +{
> + struct cdrom_multisession info;
> + int ret;
> +
> + cd_dbg(CD_DO_IOCTL, "entering CDROMMULTISESSION\n");
>
> - if (copy_to_user(argp, &ms_info, sizeof(ms_info)))
> + if (copy_from_user(&info, argp, sizeof(info)))
> + return -EFAULT;
> + ret = cdrom_multisession(cdi, &info);
> + if (ret)
> + return ret;
> + if (copy_to_user(argp, &info, sizeof(info)))
> return -EFAULT;
>
> cd_dbg(CD_DO_IOCTL, "CDROMMULTISESSION successful\n");
> - return 0;
> + return ret;
> }
>
> static int cdrom_ioctl_eject(struct cdrom_device_info *cdi)
> diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
> index 008c4d79fa33..8543fa59da72 100644
> --- a/include/linux/cdrom.h
> +++ b/include/linux/cdrom.h
> @@ -94,6 +94,8 @@ struct cdrom_device_ops {
> struct packet_command *);
> };
>
> +int cdrom_multisession(struct cdrom_device_info *cdi,
> + struct cdrom_multisession *info);
> int cdrom_read_tocentry(struct cdrom_device_info *cdi,
> struct cdrom_tocentry *entry);
>
>

Looks OK to me.

Reviewed-by: Damien Le Moal <[email protected]>

--
Damien Le Moal
Western Digital Research

2020-04-23 07:44:48

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 5/7] hfsplus: stop using ioctl_by_bdev

On 2020/04/23 16:16, Christoph Hellwig wrote:
> Instead just call the CD-ROM layer functionality directly.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/hfsplus/wrapper.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
> index 08c1580bdf7a..61eec628805d 100644
> --- a/fs/hfsplus/wrapper.c
> +++ b/fs/hfsplus/wrapper.c
> @@ -127,31 +127,34 @@ static int hfsplus_read_mdb(void *bufptr, struct hfsplus_wd *wd)
> static int hfsplus_get_last_session(struct super_block *sb,
> sector_t *start, sector_t *size)
> {
> - struct cdrom_multisession ms_info;
> - struct cdrom_tocentry te;
> - int res;
> + struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
>
> /* default values */
> *start = 0;
> *size = i_size_read(sb->s_bdev->bd_inode) >> 9;
>
> if (HFSPLUS_SB(sb)->session >= 0) {
> + struct cdrom_tocentry te;
> +
> + if (!cdi)
> + return -EINVAL;
> +
> te.cdte_track = HFSPLUS_SB(sb)->session;
> te.cdte_format = CDROM_LBA;
> - res = ioctl_by_bdev(sb->s_bdev,
> - CDROMREADTOCENTRY, (unsigned long)&te);
> - if (!res && (te.cdte_ctrl & CDROM_DATA_TRACK) == 4) {
> - *start = (sector_t)te.cdte_addr.lba << 2;
> - return 0;
> + if (cdrom_read_tocentry(cdi, &te) ||
> + (te.cdte_ctrl & CDROM_DATA_TRACK) != 4) {
> + pr_err("invalid session number or type of track\n");
> + return -EINVAL;
> }
> - pr_err("invalid session number or type of track\n");
> - return -EINVAL;
> + *start = (sector_t)te.cdte_addr.lba << 2;
> + } else if (cdi) {
> + struct cdrom_multisession ms_info;
> +
> + ms_info.addr_format = CDROM_LBA;
> + if (cdrom_multisession(cdi, &ms_info) == 0 && ms_info.xa_flag)
> + *start = (sector_t)ms_info.addr.lba << 2;
> }
> - ms_info.addr_format = CDROM_LBA;
> - res = ioctl_by_bdev(sb->s_bdev, CDROMMULTISESSION,
> - (unsigned long)&ms_info);
> - if (!res && ms_info.xa_flag)
> - *start = (sector_t)ms_info.addr.lba << 2;
> +

White space change here, but I think it's ok. I do like a blank line before
return :)

> return 0;
> }
>
>

Looks OK to me.

Reviewed-by: Damien Le Moal <[email protected]>


--
Damien Le Moal
Western Digital Research

2020-04-23 07:45:58

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 7/7] udf: stop using ioctl_by_bdev

On 2020/04/23 16:15, Christoph Hellwig wrote:
> Instead just call the CD-ROM layer functionality directly.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/udf/lowlevel.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/fs/udf/lowlevel.c b/fs/udf/lowlevel.c
> index 5c7ec121990d..f1094cdcd6cd 100644
> --- a/fs/udf/lowlevel.c
> +++ b/fs/udf/lowlevel.c
> @@ -27,41 +27,38 @@
>
> unsigned int udf_get_last_session(struct super_block *sb)
> {
> + struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
> struct cdrom_multisession ms_info;
> - unsigned int vol_desc_start;
> - struct block_device *bdev = sb->s_bdev;
> - int i;
>
> - vol_desc_start = 0;
> - ms_info.addr_format = CDROM_LBA;
> - i = ioctl_by_bdev(bdev, CDROMMULTISESSION, (unsigned long)&ms_info);
> + if (!cdi) {
> + udf_debug("CDROMMULTISESSION not supported.\n");
> + return 0;
> + }
>
> - if (i == 0) {
> + ms_info.addr_format = CDROM_LBA;
> + if (cdrom_multisession(cdi, &ms_info) == 0) {
> udf_debug("XA disk: %s, vol_desc_start=%d\n",
> ms_info.xa_flag ? "yes" : "no", ms_info.addr.lba);
> if (ms_info.xa_flag) /* necessary for a valid ms_info.addr */
> - vol_desc_start = ms_info.addr.lba;
> - } else {
> - udf_debug("CDROMMULTISESSION not supported: rc=%d\n", i);
> + return ms_info.addr.lba;
> }
> - return vol_desc_start;
> + return 0;
> }
>
> unsigned long udf_get_last_block(struct super_block *sb)
> {
> struct block_device *bdev = sb->s_bdev;
> + struct cdrom_device_info *cdi = disk_to_cdi(bdev->bd_disk);
> unsigned long lblock = 0;
>
> /*
> - * ioctl failed or returned obviously bogus value?
> + * The cdrom layer call failed or returned obviously bogus value?
> * Try using the device size...
> */
> - if (ioctl_by_bdev(bdev, CDROM_LAST_WRITTEN, (unsigned long) &lblock) ||
> - lblock == 0)
> + if (!cdi || cdrom_get_last_written(cdi, &lblock) || lblock == 0)
> lblock = i_size_read(bdev->bd_inode) >> sb->s_blocksize_bits;
>
> if (lblock)
> return lblock - 1;
> - else
> - return 0;
> + return 0;
> }
>

Looks OK to me.

Reviewed-by: Damien Le Moal <[email protected]>

--
Damien Le Moal
Western Digital Research

2020-04-23 07:47:07

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 6/7] isofs: stop using ioctl_by_bdev

On 2020/04/23 16:16, Christoph Hellwig wrote:
> Instead just call the CD-ROM layer functionality directly, and turn the
> hot mess in isofs_get_last_session into remotely readable code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/isofs/inode.c | 54 +++++++++++++++++++++++-------------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 62c0462dc89f..fc48923a9b6c 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -544,43 +544,41 @@ static int isofs_show_options(struct seq_file *m, struct dentry *root)
>
> static unsigned int isofs_get_last_session(struct super_block *sb, s32 session)
> {
> - struct cdrom_multisession ms_info;
> - unsigned int vol_desc_start;
> - struct block_device *bdev = sb->s_bdev;
> - int i;
> + struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
> + unsigned int vol_desc_start = 0;
>
> - vol_desc_start=0;
> - ms_info.addr_format=CDROM_LBA;
> if (session > 0) {
> - struct cdrom_tocentry Te;
> - Te.cdte_track=session;
> - Te.cdte_format=CDROM_LBA;
> - i = ioctl_by_bdev(bdev, CDROMREADTOCENTRY, (unsigned long) &Te);
> - if (!i) {
> + struct cdrom_tocentry te;
> +
> + if (!cdi)
> + return -EINVAL;
> +
> + te.cdte_track = session;
> + te.cdte_format = CDROM_LBA;
> + if (cdrom_read_tocentry(cdi, &te) == 0) {
> printk(KERN_DEBUG "ISOFS: Session %d start %d type %d\n",
> - session, Te.cdte_addr.lba,
> - Te.cdte_ctrl&CDROM_DATA_TRACK);
> - if ((Te.cdte_ctrl&CDROM_DATA_TRACK) == 4)
> - return Te.cdte_addr.lba;
> + session, te.cdte_addr.lba,
> + te.cdte_ctrl & CDROM_DATA_TRACK);
> + if ((te.cdte_ctrl & CDROM_DATA_TRACK) == 4)
> + return te.cdte_addr.lba;
> }
>
> printk(KERN_ERR "ISOFS: Invalid session number or type of track\n");
> }
> - i = ioctl_by_bdev(bdev, CDROMMULTISESSION, (unsigned long) &ms_info);
> - if (session > 0)
> - printk(KERN_ERR "ISOFS: Invalid session number\n");
> -#if 0
> - printk(KERN_DEBUG "isofs.inode: CDROMMULTISESSION: rc=%d\n",i);
> - if (i==0) {
> - printk(KERN_DEBUG "isofs.inode: XA disk: %s\n",ms_info.xa_flag?"yes":"no");
> - printk(KERN_DEBUG "isofs.inode: vol_desc_start = %d\n", ms_info.addr.lba);
> - }
> -#endif
> - if (i==0)
> +
> + if (cdi) {
> + struct cdrom_multisession ms_info;
> +
> + ms_info.addr_format = CDROM_LBA;
> + if (cdrom_multisession(cdi, &ms_info) == 0) {
> #if WE_OBEY_THE_WRITTEN_STANDARDS
> - if (ms_info.xa_flag) /* necessary for a valid ms_info.addr */
> + /* necessary for a valid ms_info.addr */
> + if (ms_info.xa_flag)
> #endif
> - vol_desc_start=ms_info.addr.lba;
> + vol_desc_start = ms_info.addr.lba;
> + }
> + }
> +
> return vol_desc_start;
> }
>
>

Looks OK to me.

Reviewed-by: Damien Le Moal <[email protected]>

--
Damien Le Moal
Western Digital Research

2020-04-23 11:05:44

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 6/7] isofs: stop using ioctl_by_bdev

On Thu 23-04-20 09:12:23, Christoph Hellwig wrote:
> Instead just call the CD-ROM layer functionality directly, and turn the
> hot mess in isofs_get_last_session into remotely readable code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

One comment below...

> ---
> fs/isofs/inode.c | 54 +++++++++++++++++++++++-------------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 62c0462dc89f..fc48923a9b6c 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -544,43 +544,41 @@ static int isofs_show_options(struct seq_file *m, struct dentry *root)
>
> static unsigned int isofs_get_last_session(struct super_block *sb, s32 session)
> {
> - struct cdrom_multisession ms_info;
> - unsigned int vol_desc_start;
> - struct block_device *bdev = sb->s_bdev;
> - int i;
> + struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
> + unsigned int vol_desc_start = 0;
>
> - vol_desc_start=0;
> - ms_info.addr_format=CDROM_LBA;
> if (session > 0) {
> - struct cdrom_tocentry Te;
> - Te.cdte_track=session;
> - Te.cdte_format=CDROM_LBA;
> - i = ioctl_by_bdev(bdev, CDROMREADTOCENTRY, (unsigned long) &Te);
> - if (!i) {
> + struct cdrom_tocentry te;
> +
> + if (!cdi)
> + return -EINVAL;

There's no error handling in the caller and this function actually returns
unsigned int... So I believe you need to return 0 here to maintain previous
behavior (however suspicious it may be)?

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2020-04-23 11:07:50

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 7/7] udf: stop using ioctl_by_bdev

On Thu 23-04-20 09:12:24, Christoph Hellwig wrote:
> Instead just call the CD-ROM layer functionality directly.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/udf/lowlevel.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/fs/udf/lowlevel.c b/fs/udf/lowlevel.c
> index 5c7ec121990d..f1094cdcd6cd 100644
> --- a/fs/udf/lowlevel.c
> +++ b/fs/udf/lowlevel.c
> @@ -27,41 +27,38 @@
>
> unsigned int udf_get_last_session(struct super_block *sb)
> {
> + struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
> struct cdrom_multisession ms_info;
> - unsigned int vol_desc_start;
> - struct block_device *bdev = sb->s_bdev;
> - int i;
>
> - vol_desc_start = 0;
> - ms_info.addr_format = CDROM_LBA;
> - i = ioctl_by_bdev(bdev, CDROMMULTISESSION, (unsigned long)&ms_info);
> + if (!cdi) {
> + udf_debug("CDROMMULTISESSION not supported.\n");
> + return 0;
> + }
>
> - if (i == 0) {
> + ms_info.addr_format = CDROM_LBA;
> + if (cdrom_multisession(cdi, &ms_info) == 0) {
> udf_debug("XA disk: %s, vol_desc_start=%d\n",
> ms_info.xa_flag ? "yes" : "no", ms_info.addr.lba);
> if (ms_info.xa_flag) /* necessary for a valid ms_info.addr */
> - vol_desc_start = ms_info.addr.lba;
> - } else {
> - udf_debug("CDROMMULTISESSION not supported: rc=%d\n", i);
> + return ms_info.addr.lba;
> }
> - return vol_desc_start;
> + return 0;
> }
>
> unsigned long udf_get_last_block(struct super_block *sb)
> {
> struct block_device *bdev = sb->s_bdev;
> + struct cdrom_device_info *cdi = disk_to_cdi(bdev->bd_disk);
> unsigned long lblock = 0;
>
> /*
> - * ioctl failed or returned obviously bogus value?
> + * The cdrom layer call failed or returned obviously bogus value?
> * Try using the device size...
> */
> - if (ioctl_by_bdev(bdev, CDROM_LAST_WRITTEN, (unsigned long) &lblock) ||
> - lblock == 0)
> + if (!cdi || cdrom_get_last_written(cdi, &lblock) || lblock == 0)
> lblock = i_size_read(bdev->bd_inode) >> sb->s_blocksize_bits;
>
> if (lblock)
> return lblock - 1;
> - else
> - return 0;
> + return 0;
> }
> --
> 2.26.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2020-04-24 06:57:07

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 6/7] isofs: stop using ioctl_by_bdev

On Thu, Apr 23, 2020 at 01:03:47PM +0200, Jan Kara wrote:
> There's no error handling in the caller and this function actually returns
> unsigned int... So I believe you need to return 0 here to maintain previous
> behavior (however suspicious it may be)?

Indeed, and I don't think it is suspicious at all - if we have no CDROM
info we should assume session 0, which is the same as for non-CDROM
devices. Fixed for the next version.

2020-04-24 09:23:19

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 6/7] isofs: stop using ioctl_by_bdev

On Fri 24-04-20 08:52:53, Christoph Hellwig wrote:
> On Thu, Apr 23, 2020 at 01:03:47PM +0200, Jan Kara wrote:
> > There's no error handling in the caller and this function actually returns
> > unsigned int... So I believe you need to return 0 here to maintain previous
> > behavior (however suspicious it may be)?
>
> Indeed, and I don't think it is suspicious at all - if we have no CDROM
> info we should assume session 0, which is the same as for non-CDROM
> devices. Fixed for the next version.

Right, I've realized that once I've checked UDF version and thought about
it for a while...

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR