[PATCH v4 rebase 00/10] Fix cdrom autoclose

[PATCH v4 rebase 00/10] Fix cdrom autoclose

[Michal Suchanek]

Hello,

there is cdrom autoclose feature that is supposed to close the tray,
wait for the disc to become ready, and then open the device.

This used to work in ancient times. Then in old times there was a hack
in util-linux which worked around the breakage which probably resulted
from switching to scsi emulation.

Currently util-linux maintainer refuses to merge another hack on the
basis that kernel still has the feature so it should be fixed there.
The code needs not be replicated in every userspace utility like mount
or dd which has no business knowing which devices are CD-roms and where
the autoclose setting is in the kernel.

This is rebase on top of current master.

Also it seems that most people think that this is fix for WMware because
there is one patch dealing with WMware.

This is fix for Linux.

Expected (ca Linux 2.4):

eject
< put CD on tray >
mount /dev/cdrom
...
< cdrom now mounted >

Actual:
mount: /mnt: no medium found on /dev/sr0.

Thanks

Michal

v3:
- change the VMware workaround to use blacklist flag
- use exported function instead of ioctl
v4:
- fix crash reported by kernel test robot
- fix the debug message logic while refactoring cdrom_open
- move repeated code out of __blkdev_get

Link: https://lore.kernel.org/lkml/cover.1571834862.git.msuchanek@suse.de/
Link: https://lore.kernel.org/lkml/cover.1513263482.git.msuchanek@suse.de/

Michal Suchanek (10):
  cdrom: add poll_event_interruptible
  cdrom: factor out common open_for_* code
  cdrom: wait for the tray to close
  cdrom: export autoclose logic as a separate function
  cdrom: unify log messages.
  bdev: reset first_open when looping in __blkget_dev
  bdev: separate parts of __blkdev_get as helper functions
  bdev: add open_finish
  scsi: blacklist: add VMware ESXi cdrom - broken tray emulation
  scsi: sr: wait for the medium to become ready

 Documentation/filesystems/locking.rst |   2 +
 drivers/cdrom/cdrom.c                 | 471 +++++++++++++-------------
 drivers/scsi/scsi_devinfo.c           |  15 +-
 drivers/scsi/sr.c                     |  60 +++-
 fs/block_dev.c                        |  72 ++--
 include/linux/blkdev.h                |   1 +
 include/linux/cdrom.h                 |   1 +
 include/scsi/scsi_devinfo.h           |   7 +-
 8 files changed, 357 insertions(+), 272 deletions(-)

-- 
2.23.0

[PATCH v4 rebase 01/10] cdrom: add poll_event_interruptible

[Michal Suchanek]

Add convenience macro for polling an event that does not have a
waitqueue.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 drivers/cdrom/cdrom.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index ac42ae4651ce..2ad6b73482fe 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -282,10 +282,24 @@
 #include <linux/fcntl.h>
 #include <linux/blkdev.h>
 #include <linux/times.h>
+#include <linux/delay.h>
 #include <linux/uaccess.h>
+#include <linux/sched/signal.h>
 #include <scsi/scsi_common.h>
 #include <scsi/scsi_request.h>
 
+#define poll_event_interruptible(event, interval) ({ \
+	int ret = 0; \
+	while (!(event)) { \
+		if (signal_pending(current)) { \
+			ret = -ERESTARTSYS; \
+			break; \
+		} \
+		msleep_interruptible(interval); \
+	} \
+	ret; \
+})
+
 /* used to tell the module to turn on full debugging messages */
 static bool debug;
 /* default compatibility mode */
-- 
2.23.0

[PATCH v4 rebase 02/10] cdrom: factor out common open_for_* code

[Michal Suchanek]

The open_for_audio and open_for_data copies are bitrotten in different
ways already and will need to update the autoclose logic in both.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: do not add unrelated whitespace changes
v3: fix declaration style
v4: fix the debug message logic around closing tray
---
 drivers/cdrom/cdrom.c | 118 ++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 78 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 2ad6b73482fe..bb32decdadf1 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1045,13 +1045,12 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
 	       tracks->cdi, tracks->xa);
 }
 
-static
-int open_for_data(struct cdrom_device_info *cdi)
+static int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
 {
 	int ret;
 	const struct cdrom_device_ops *cdo = cdi->ops;
-	tracktype tracks;
-	cd_dbg(CD_OPEN, "entering open_for_data\n");
+
+	cd_dbg(CD_OPEN, "entering open_for_common\n");
 	/* Check if the driver can report drive status.  If it can, we
 	   can do clever things.  If it can't, well, we at least tried! */
 	if (cdo->drive_status != NULL) {
@@ -1059,49 +1058,48 @@ int open_for_data(struct cdrom_device_info *cdi)
 		cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
 		if (ret == CDS_TRAY_OPEN) {
 			cd_dbg(CD_OPEN, "the tray is open...\n");
-			/* can/may i close it? */
-			if (CDROM_CAN(CDC_CLOSE_TRAY) &&
-			    cdi->options & CDO_AUTO_CLOSE) {
+			if (CDROM_CAN(CDC_CLOSE_TRAY)) {
+				if (!(cdi->options & CDO_AUTO_CLOSE))
+					return -ENOMEDIUM;
 				cd_dbg(CD_OPEN, "trying to close the tray\n");
 				ret=cdo->tray_move(cdi,0);
 				if (ret) {
 					cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
-					/* Ignore the error from the low
-					level driver.  We don't care why it
-					couldn't close the tray.  We only care 
-					that there is no disc in the drive, 
-					since that is the _REAL_ problem here.*/
-					ret=-ENOMEDIUM;
-					goto clean_up_and_return;
+					return -ENOMEDIUM;
 				}
+				ret = cdo->drive_status(cdi, CDSL_CURRENT);
+				if (ret == CDS_TRAY_OPEN)
+					cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
+				if (ret == CDS_NO_DISC)
+					cd_dbg(CD_OPEN, "tray might not contain a medium\n");
 			} else {
 				cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
-				ret=-ENOMEDIUM;
-				goto clean_up_and_return;
-			}
-			/* Ok, the door should be closed now.. Check again */
-			ret = cdo->drive_status(cdi, CDSL_CURRENT);
-			if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
-				cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
-				cd_dbg(CD_OPEN, "tray might not contain a medium\n");
-				ret=-ENOMEDIUM;
-				goto clean_up_and_return;
+				return -ENOMEDIUM;
 			}
-			cd_dbg(CD_OPEN, "the tray is now closed\n");
-		}
-		/* the door should be closed now, check for the disc */
-		ret = cdo->drive_status(cdi, CDSL_CURRENT);
-		if (ret!=CDS_DISC_OK) {
-			ret = -ENOMEDIUM;
-			goto clean_up_and_return;
 		}
+		if (ret != CDS_DISC_OK)
+			return -ENOMEDIUM;
 	}
-	cdrom_count_tracks(cdi, &tracks);
-	if (tracks.error == CDS_NO_DISC) {
+	cdrom_count_tracks(cdi, tracks);
+	if (tracks->error == CDS_NO_DISC) {
 		cd_dbg(CD_OPEN, "bummer. no disc.\n");
-		ret=-ENOMEDIUM;
-		goto clean_up_and_return;
+		return -ENOMEDIUM;
 	}
+
+	return 0;
+}
+
+static int open_for_data(struct cdrom_device_info *cdi)
+{
+	int ret;
+	const struct cdrom_device_ops *cdo = cdi->ops;
+	tracktype tracks;
+
+	cd_dbg(CD_OPEN, "entering open_for_data\n");
+	ret = open_for_common(cdi, &tracks);
+	if (ret)
+		goto clean_up_and_return;
+
 	/* CD-Players which don't use O_NONBLOCK, workman
 	 * for example, need bit CDO_CHECK_TYPE cleared! */
 	if (tracks.data==0) {
@@ -1208,53 +1206,17 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
 /* This code is similar to that in open_for_data. The routine is called
    whenever an audio play operation is requested.
 */
-static int check_for_audio_disc(struct cdrom_device_info *cdi,
-				const struct cdrom_device_ops *cdo)
+static int check_for_audio_disc(struct cdrom_device_info *cdi)
 {
         int ret;
 	tracktype tracks;
 	cd_dbg(CD_OPEN, "entering check_for_audio_disc\n");
 	if (!(cdi->options & CDO_CHECK_TYPE))
 		return 0;
-	if (cdo->drive_status != NULL) {
-		ret = cdo->drive_status(cdi, CDSL_CURRENT);
-		cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
-		if (ret == CDS_TRAY_OPEN) {
-			cd_dbg(CD_OPEN, "the tray is open...\n");
-			/* can/may i close it? */
-			if (CDROM_CAN(CDC_CLOSE_TRAY) &&
-			    cdi->options & CDO_AUTO_CLOSE) {
-				cd_dbg(CD_OPEN, "trying to close the tray\n");
-				ret=cdo->tray_move(cdi,0);
-				if (ret) {
-					cd_dbg(CD_OPEN, "bummer. tried to close tray but failed.\n");
-					/* Ignore the error from the low
-					level driver.  We don't care why it
-					couldn't close the tray.  We only care 
-					that there is no disc in the drive, 
-					since that is the _REAL_ problem here.*/
-					return -ENOMEDIUM;
-				}
-			} else {
-				cd_dbg(CD_OPEN, "bummer. this driver can't close the tray.\n");
-				return -ENOMEDIUM;
-			}
-			/* Ok, the door should be closed now.. Check again */
-			ret = cdo->drive_status(cdi, CDSL_CURRENT);
-			if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
-				cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
-				return -ENOMEDIUM;
-			}	
-			if (ret!=CDS_DISC_OK) {
-				cd_dbg(CD_OPEN, "bummer. disc isn't ready.\n");
-				return -EIO;
-			}	
-			cd_dbg(CD_OPEN, "the tray is now closed\n");
-		}	
-	}
-	cdrom_count_tracks(cdi, &tracks);
-	if (tracks.error) 
-		return(tracks.error);
+
+	ret = open_for_common(cdi, &tracks);
+	if (ret)
+		return ret;
 
 	if (tracks.audio==0)
 		return -EMEDIUMTYPE;
@@ -2725,7 +2687,7 @@ static int cdrom_ioctl_play_trkind(struct cdrom_device_info *cdi,
 	if (copy_from_user(&ti, argp, sizeof(ti)))
 		return -EFAULT;
 
-	ret = check_for_audio_disc(cdi, cdi->ops);
+	ret = check_for_audio_disc(cdi);
 	if (ret)
 		return ret;
 	return cdi->ops->audio_ioctl(cdi, CDROMPLAYTRKIND, &ti);
@@ -2773,7 +2735,7 @@ static int cdrom_ioctl_audioctl(struct cdrom_device_info *cdi,
 
 	if (!CDROM_CAN(CDC_PLAY_AUDIO))
 		return -ENOSYS;
-	ret = check_for_audio_disc(cdi, cdi->ops);
+	ret = check_for_audio_disc(cdi);
 	if (ret)
 		return ret;
 	return cdi->ops->audio_ioctl(cdi, cmd, NULL);
-- 
2.23.0

[PATCH v4 rebase 03/10] cdrom: wait for the tray to close

[Michal Suchanek]

The scsi command to close the tray only starts the motor and does not
wait for the tray to close. Wait until the state chages from TRAY_OPEN
so users do not race with the tray closing.

This looks like inifinte wait but unless the drive is broken it either
closes the tray shortly or reports an error when it detects the tray is
blocked. At worst the wait can be interrupted by the user.

Also wait for the drive to become ready once the tray closes.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2:
 - check drive_status exists before using it
 - rename tray_close -> cdrom_tray_close
 - also wait for drive to become ready after tray closes
 - do not wait in cdrom_ioctl_closetray
v4: add a status change poll wrapper
---
 drivers/cdrom/cdrom.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index bb32decdadf1..b2bc0c8f9a69 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1045,6 +1045,28 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
 	       tracks->cdi, tracks->xa);
 }
 
+static int cdrom_wait_for_status_change(struct cdrom_device_info *cdi,
+					int *status)
+{
+	int saved = *status;
+	const struct cdrom_device_ops *cdo = cdi->ops;
+
+	return poll_event_interruptible(saved !=
+			(*status = cdo->drive_status(cdi, CDSL_CURRENT)), 500);
+}
+
+static int cdrom_tray_close(struct cdrom_device_info *cdi)
+{
+	int ret;
+
+	ret = cdi->ops->tray_move(cdi, 0);
+	if (ret || !cdi->ops->drive_status)
+		return ret;
+
+	ret = CDS_TRAY_OPEN;
+	return cdrom_wait_for_status_change(cdi, &ret);
+}
+
 static int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
 {
 	int ret;
@@ -1062,14 +1084,14 @@ static int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
 				if (!(cdi->options & CDO_AUTO_CLOSE))
 					return -ENOMEDIUM;
 				cd_dbg(CD_OPEN, "trying to close the tray\n");
-				ret=cdo->tray_move(cdi,0);
+				ret = cdrom_tray_close(cdi);
+				if (ret == -ERESTARTSYS)
+					return ret;
 				if (ret) {
 					cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
 					return -ENOMEDIUM;
 				}
 				ret = cdo->drive_status(cdi, CDSL_CURRENT);
-				if (ret == CDS_TRAY_OPEN)
-					cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
 				if (ret == CDS_NO_DISC)
 					cd_dbg(CD_OPEN, "tray might not contain a medium\n");
 			} else {
@@ -1077,6 +1099,14 @@ static int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
 				return -ENOMEDIUM;
 			}
 		}
+		if (ret == CDS_DRIVE_NOT_READY) {
+			int poll_res;
+
+			cd_dbg(CD_OPEN, "waiting for drive to become ready...\n");
+			poll_res = cdrom_wait_for_status_change(cdi, &ret);
+			if (poll_res == -ERESTARTSYS)
+				return poll_res;
+		}
 		if (ret != CDS_DISC_OK)
 			return -ENOMEDIUM;
 	}
-- 
2.23.0

[PATCH v4 rebase 04/10] cdrom: export autoclose logic as a separate function

[Michal Suchanek]

This allows the sr driver to call it without blocking other processes
accessing the device. This solves a issue with process waiting in open()
on broken drive to close the tray blocking out all access to the device,
including nonblocking access to determine drive status.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: new patch
v3:
 - change IOCTL to export
 - fix the logic about reporting cdroms inability to close tray
v4:
 - move the autoclose code to the open code, align debug messages with
 the open code
---
 drivers/cdrom/cdrom.c | 85 ++++++++++++++++++++++++++++++-------------
 include/linux/cdrom.h |  1 +
 2 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index b2bc0c8f9a69..38e6db879de0 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1067,6 +1067,56 @@ static int cdrom_tray_close(struct cdrom_device_info *cdi)
 	return cdrom_wait_for_status_change(cdi, &ret);
 }
 
+int cdrom_autoclose(struct cdrom_device_info *cdi)
+{
+	int ret;
+	const struct cdrom_device_ops *cdo = cdi->ops;
+
+	cd_dbg(CD_OPEN, "entering cdrom_autoclose\n");
+	if (!cdo->drive_status)
+		return -EOPNOTSUPP;
+
+	ret = cdo->drive_status(cdi, CDSL_CURRENT);
+	cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
+
+	if (ret == CDS_TRAY_OPEN) {
+		cd_dbg(CD_OPEN, "the tray is open...\n");
+		if (CDROM_CAN(CDC_CLOSE_TRAY)) {
+			if (!(cdi->options & CDO_AUTO_CLOSE))
+				return -ENOMEDIUM;
+			cd_dbg(CD_OPEN, "trying to close the tray\n");
+			ret = cdrom_tray_close(cdi);
+			if (ret == -ERESTARTSYS)
+				return ret;
+			if (ret) {
+				cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
+				return -ENOMEDIUM;
+			}
+			ret = cdo->drive_status(cdi, CDSL_CURRENT);
+			if (ret == CDS_NO_DISC)
+				cd_dbg(CD_OPEN, "tray might not contain a medium\n");
+		} else {
+			cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
+			return -ENOMEDIUM;
+		}
+	}
+
+	if (ret == CDS_DRIVE_NOT_READY) {
+		int poll_res;
+
+		cd_dbg(CD_OPEN, "waiting for drive to become ready...\n");
+		poll_res = cdrom_wait_for_status_change(cdi, &ret);
+		if (poll_res == -ERESTARTSYS)
+			return poll_res;
+	}
+
+	if (ret != CDS_DISC_OK)
+		return -ENOMEDIUM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cdrom_autoclose);
+
 static int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
 {
 	int ret;
@@ -1080,35 +1130,20 @@ static int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
 		cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
 		if (ret == CDS_TRAY_OPEN) {
 			cd_dbg(CD_OPEN, "the tray is open...\n");
-			if (CDROM_CAN(CDC_CLOSE_TRAY)) {
-				if (!(cdi->options & CDO_AUTO_CLOSE))
-					return -ENOMEDIUM;
-				cd_dbg(CD_OPEN, "trying to close the tray\n");
-				ret = cdrom_tray_close(cdi);
-				if (ret == -ERESTARTSYS)
-					return ret;
-				if (ret) {
-					cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
-					return -ENOMEDIUM;
-				}
-				ret = cdo->drive_status(cdi, CDSL_CURRENT);
-				if (ret == CDS_NO_DISC)
-					cd_dbg(CD_OPEN, "tray might not contain a medium\n");
-			} else {
-				cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
-				return -ENOMEDIUM;
-			}
+			return -ENOMEDIUM;
 		}
 		if (ret == CDS_DRIVE_NOT_READY) {
-			int poll_res;
-
-			cd_dbg(CD_OPEN, "waiting for drive to become ready...\n");
-			poll_res = cdrom_wait_for_status_change(cdi, &ret);
-			if (poll_res == -ERESTARTSYS)
-				return poll_res;
+			cd_dbg(CD_OPEN, "the drive is not ready...\n");
+			return -ENOMEDIUM;
 		}
-		if (ret != CDS_DISC_OK)
+		if (ret == CDS_NO_DISC) {
+			cd_dbg(CD_OPEN, "tray might not contain a medium...\n");
 			return -ENOMEDIUM;
+		}
+		if (ret != CDS_DISC_OK) {
+			cd_dbg(CD_OPEN, "drive returned status %i...\n", ret);
+			return -ENOMEDIUM;
+		}
 	}
 	cdrom_count_tracks(cdi, tracks);
 	if (tracks->error == CDS_NO_DISC) {
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 528271c60018..ea0415ba9f9d 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -126,6 +126,7 @@ extern void init_cdrom_command(struct packet_command *cgc,
 			       void *buffer, int len, int type);
 extern int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
 				      struct packet_command *cgc);
+int cdrom_autoclose(struct cdrom_device_info *cdi);
 
 /* The SCSI spec says there could be 256 slots. */
 #define CDROM_MAX_SLOTS	256
-- 
2.23.0

[PATCH v4 rebase 05/10] cdrom: unify log messages.

[Michal Suchanek]

Remove obsolete compile time debug print settings, cd_dbg uses pr_debug
which can be dynamically enabled anyway.

Always print device name in cd_dbg.

Add cd_err and cd_info macros to always print device name in messages.

Remove redundant device name in message format string.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 drivers/cdrom/cdrom.c | 286 +++++++++++++++++++-----------------------
 1 file changed, 128 insertions(+), 158 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 38e6db879de0..93c9cea53f51 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -247,24 +247,6 @@
 #define REVISION "Revision: 3.20"
 #define VERSION "Id: cdrom.c 3.20 2003/12/17"
 
-/* I use an error-log mask to give fine grain control over the type of
-   messages dumped to the system logs.  The available masks include: */
-#define CD_NOTHING      0x0
-#define CD_WARNING	0x1
-#define CD_REG_UNREG	0x2
-#define CD_DO_IOCTL	0x4
-#define CD_OPEN		0x8
-#define CD_CLOSE	0x10
-#define CD_COUNT_TRACKS 0x20
-#define CD_CHANGER	0x40
-#define CD_DVD		0x80
-
-/* Define this to remove _all_ the debugging messages */
-/* #define ERRLOGMASK CD_NOTHING */
-#define ERRLOGMASK CD_WARNING
-/* #define ERRLOGMASK (CD_WARNING|CD_OPEN|CD_COUNT_TRACKS|CD_CLOSE) */
-/* #define ERRLOGMASK (CD_WARNING|CD_REG_UNREG|CD_DO_IOCTL|CD_OPEN|CD_CLOSE|CD_COUNT_TRACKS) */
-
 #include <linux/atomic.h>
 #include <linux/module.h>
 #include <linux/fs.h>
@@ -328,19 +310,14 @@ static const char *mrw_format_status[] = {
 
 static const char *mrw_address_space[] = { "DMA", "GAA" };
 
-#if (ERRLOGMASK != CD_NOTHING)
-#define cd_dbg(type, fmt, ...)				\
-do {							\
-	if ((ERRLOGMASK & type) || debug == 1)		\
-		pr_debug(fmt, ##__VA_ARGS__);		\
-} while (0)
-#else
-#define cd_dbg(type, fmt, ...)				\
-do {							\
-	if (0 && (ERRLOGMASK & type) || debug == 1)	\
-		pr_debug(fmt, ##__VA_ARGS__);		\
-} while (0)
-#endif
+#define cd_dbg(cdi, fmt, ...)						 \
+	pr_debug("%s: " fmt, (cdi) ? (cdi)->name : NULL, ##__VA_ARGS__)	 \
+
+#define cd_info(cdi, fmt, ...)						 \
+	pr_info("%s: " fmt, (cdi) ? (cdi)->name : NULL, ##__VA_ARGS__)	 \
+
+#define cd_err(cdi, fmt, ...)						 \
+	pr_err("%s: " fmt, (cdi) ? (cdi)->name : NULL, ##__VA_ARGS__)	 \
 
 /* The (cdo->capability & ~cdi->mask & CDC_XXX) construct was used in
    a lot of places. This macro makes the code more clear. */
@@ -494,7 +471,7 @@ static int cdrom_mrw_bgformat(struct cdrom_device_info *cdi, int cont)
 	unsigned char buffer[12];
 	int ret;
 
-	pr_info("%sstarting format\n", cont ? "Re" : "");
+	cd_info(cdi, "%sstarting format\n", cont ? "Re" : "");
 
 	/*
 	 * FmtData bit set (bit 4), format type is 1
@@ -524,7 +501,7 @@ static int cdrom_mrw_bgformat(struct cdrom_device_info *cdi, int cont)
 
 	ret = cdi->ops->generic_packet(cdi, &cgc);
 	if (ret)
-		pr_info("bgformat failed\n");
+		cd_info(cdi, "bgformat failed\n");
 
 	return ret;
 }
@@ -558,7 +535,7 @@ static int cdrom_mrw_exit(struct cdrom_device_info *cdi)
 
 	ret = 0;
 	if (di.mrw_status == CDM_MRW_BGFORMAT_ACTIVE) {
-		pr_info("issuing MRW background format suspend\n");
+		cd_info(cdi, "issuing MRW background format suspend\n");
 		ret = cdrom_mrw_bgformat_susp(cdi, 0);
 	}
 
@@ -595,8 +572,8 @@ static int cdrom_mrw_set_lba_space(struct cdrom_device_info *cdi, int space)
 	if (ret)
 		return ret;
 
-	pr_info("%s: mrw address space %s selected\n",
-		cdi->name, mrw_address_space[space]);
+	cd_info(cdi, "mrw address space %s selected\n",
+		mrw_address_space[space]);
 	return 0;
 }
 
@@ -605,7 +582,7 @@ int register_cdrom(struct cdrom_device_info *cdi)
 	static char banner_printed;
 	const struct cdrom_device_ops *cdo = cdi->ops;
 
-	cd_dbg(CD_OPEN, "entering register_cdrom\n");
+	cd_dbg(cdi, "entering register_cdrom\n");
 
 	if (cdo->open == NULL || cdo->release == NULL)
 		return -EINVAL;
@@ -647,7 +624,7 @@ int register_cdrom(struct cdrom_device_info *cdi)
 
 	WARN_ON(!cdo->generic_packet);
 
-	cd_dbg(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
+	cd_dbg(cdi, "drive registered\n");
 	mutex_lock(&cdrom_mutex);
 	list_add(&cdi->list, &cdrom_list);
 	mutex_unlock(&cdrom_mutex);
@@ -657,7 +634,7 @@ int register_cdrom(struct cdrom_device_info *cdi)
 
 void unregister_cdrom(struct cdrom_device_info *cdi)
 {
-	cd_dbg(CD_OPEN, "entering unregister_cdrom\n");
+	cd_dbg(cdi, "entering unregister_cdrom\n");
 
 	mutex_lock(&cdrom_mutex);
 	list_del(&cdi->list);
@@ -666,7 +643,7 @@ void unregister_cdrom(struct cdrom_device_info *cdi)
 	if (cdi->exit)
 		cdi->exit(cdi);
 
-	cd_dbg(CD_REG_UNREG, "drive \"/dev/%s\" unregistered\n", cdi->name);
+	cd_dbg(cdi, "drive unregistered\n");
 }
 
 int cdrom_get_media_event(struct cdrom_device_info *cdi,
@@ -796,7 +773,7 @@ static int cdrom_mrw_open_write(struct cdrom_device_info *cdi)
 	 * always reset to DMA lba space on open
 	 */
 	if (cdrom_mrw_set_lba_space(cdi, MRW_LBA_DMA)) {
-		pr_err("failed setting lba address space\n");
+		cd_err(cdi, "failed setting lba address space\n");
 		return 1;
 	}
 
@@ -815,7 +792,8 @@ static int cdrom_mrw_open_write(struct cdrom_device_info *cdi)
 	 * 3	-	MRW formatting complete
 	 */
 	ret = 0;
-	pr_info("open: mrw_status '%s'\n", mrw_format_status[di.mrw_status]);
+	cd_info(cdi, "open: mrw_status '%s'\n",
+		mrw_format_status[di.mrw_status]);
 	if (!di.mrw_status)
 		ret = 1;
 	else if (di.mrw_status == CDM_MRW_BGFORMAT_INACTIVE &&
@@ -867,7 +845,7 @@ static int cdrom_ram_open_write(struct cdrom_device_info *cdi)
 	else if (CDF_RWRT == be16_to_cpu(rfd.feature_code))
 		ret = !rfd.curr;
 
-	cd_dbg(CD_OPEN, "can open for random write\n");
+	cd_dbg(cdi, "can open for random write\n");
 	return ret;
 }
 
@@ -957,16 +935,16 @@ static void cdrom_dvd_rw_close_write(struct cdrom_device_info *cdi)
 	struct packet_command cgc;
 
 	if (cdi->mmc3_profile != 0x1a) {
-		cd_dbg(CD_CLOSE, "%s: No DVD+RW\n", cdi->name);
+		cd_dbg(cdi, "No DVD+RW\n");
 		return;
 	}
 
 	if (!cdi->media_written) {
-		cd_dbg(CD_CLOSE, "%s: DVD+RW media clean\n", cdi->name);
+		cd_dbg(cdi, "DVD+RW media clean\n");
 		return;
 	}
 
-	pr_info("%s: dirty DVD+RW media, \"finalizing\"\n", cdi->name);
+	cd_info(cdi, "dirty DVD+RW media, \"finalizing\"\n");
 
 	init_cdrom_command(&cgc, NULL, 0, CGC_DATA_NONE);
 	cgc.cmd[0] = GPCMD_FLUSH_CACHE;
@@ -1009,7 +987,7 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
 	tracks->cdi = 0;
 	tracks->xa = 0;
 	tracks->error = 0;
-	cd_dbg(CD_COUNT_TRACKS, "entering cdrom_count_tracks\n");
+	cd_dbg(cdi, "entering cdrom_count_tracks\n");
 	/* Grab the TOC header so we can see how many tracks there are */
 	ret = cdi->ops->audio_ioctl(cdi, CDROMREADTOCHDR, &header);
 	if (ret) {
@@ -1037,10 +1015,10 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
 		} else {
 			tracks->audio++;
 		}
-		cd_dbg(CD_COUNT_TRACKS, "track %d: format=%d, ctrl=%d\n",
+		cd_dbg(cdi, "track %d: format=%d, ctrl=%d\n",
 		       i, entry.cdte_format, entry.cdte_ctrl);
 	}
-	cd_dbg(CD_COUNT_TRACKS, "disc has %d tracks: %d=audio %d=data %d=Cd-I %d=XA\n",
+	cd_dbg(cdi, "disc has %d tracks: %d=audio %d=data %d=Cd-I %d=XA\n",
 	       header.cdth_trk1, tracks->audio, tracks->data,
 	       tracks->cdi, tracks->xa);
 }
@@ -1072,31 +1050,31 @@ int cdrom_autoclose(struct cdrom_device_info *cdi)
 	int ret;
 	const struct cdrom_device_ops *cdo = cdi->ops;
 
-	cd_dbg(CD_OPEN, "entering cdrom_autoclose\n");
+	cd_dbg(cdi, "entering cdrom_autoclose\n");
 	if (!cdo->drive_status)
 		return -EOPNOTSUPP;
 
 	ret = cdo->drive_status(cdi, CDSL_CURRENT);
-	cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
+	cd_dbg(cdi, "drive_status=%d\n", ret);
 
 	if (ret == CDS_TRAY_OPEN) {
-		cd_dbg(CD_OPEN, "the tray is open...\n");
+		cd_dbg(cdi, "the tray is open...\n");
 		if (CDROM_CAN(CDC_CLOSE_TRAY)) {
 			if (!(cdi->options & CDO_AUTO_CLOSE))
 				return -ENOMEDIUM;
-			cd_dbg(CD_OPEN, "trying to close the tray\n");
+			cd_dbg(cdi, "trying to close the tray\n");
 			ret = cdrom_tray_close(cdi);
 			if (ret == -ERESTARTSYS)
 				return ret;
 			if (ret) {
-				cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
+				cd_dbg(cdi, "bummer. tried to close the tray but failed.\n");
 				return -ENOMEDIUM;
 			}
 			ret = cdo->drive_status(cdi, CDSL_CURRENT);
 			if (ret == CDS_NO_DISC)
-				cd_dbg(CD_OPEN, "tray might not contain a medium\n");
+				cd_dbg(cdi, "tray might not contain a medium\n");
 		} else {
-			cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
+			cd_dbg(cdi, "bummer. this drive can't close the tray.\n");
 			return -ENOMEDIUM;
 		}
 	}
@@ -1104,7 +1082,7 @@ int cdrom_autoclose(struct cdrom_device_info *cdi)
 	if (ret == CDS_DRIVE_NOT_READY) {
 		int poll_res;
 
-		cd_dbg(CD_OPEN, "waiting for drive to become ready...\n");
+		cd_dbg(cdi, "waiting for drive to become ready...\n");
 		poll_res = cdrom_wait_for_status_change(cdi, &ret);
 		if (poll_res == -ERESTARTSYS)
 			return poll_res;
@@ -1122,32 +1100,32 @@ static int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
 	int ret;
 	const struct cdrom_device_ops *cdo = cdi->ops;
 
-	cd_dbg(CD_OPEN, "entering open_for_common\n");
+	cd_dbg(cdi, "entering open_for_common\n");
 	/* Check if the driver can report drive status.  If it can, we
 	   can do clever things.  If it can't, well, we at least tried! */
 	if (cdo->drive_status != NULL) {
 		ret = cdo->drive_status(cdi, CDSL_CURRENT);
-		cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
+		cd_dbg(cdi, "drive_status=%d\n", ret);
 		if (ret == CDS_TRAY_OPEN) {
-			cd_dbg(CD_OPEN, "the tray is open...\n");
+			cd_dbg(cdi, "the tray is open...\n");
 			return -ENOMEDIUM;
 		}
 		if (ret == CDS_DRIVE_NOT_READY) {
-			cd_dbg(CD_OPEN, "the drive is not ready...\n");
+			cd_dbg(cdi, "the drive is not ready...\n");
 			return -ENOMEDIUM;
 		}
 		if (ret == CDS_NO_DISC) {
-			cd_dbg(CD_OPEN, "tray might not contain a medium...\n");
+			cd_dbg(cdi, "tray might not contain a medium...\n");
 			return -ENOMEDIUM;
 		}
 		if (ret != CDS_DISC_OK) {
-			cd_dbg(CD_OPEN, "drive returned status %i...\n", ret);
+			cd_dbg(cdi, "drive returned status %i...\n", ret);
 			return -ENOMEDIUM;
 		}
 	}
 	cdrom_count_tracks(cdi, tracks);
 	if (tracks->error == CDS_NO_DISC) {
-		cd_dbg(CD_OPEN, "bummer. no disc.\n");
+		cd_dbg(cdi, "bummer. no disc.\n");
 		return -ENOMEDIUM;
 	}
 
@@ -1160,7 +1138,7 @@ static int open_for_data(struct cdrom_device_info *cdi)
 	const struct cdrom_device_ops *cdo = cdi->ops;
 	tracktype tracks;
 
-	cd_dbg(CD_OPEN, "entering open_for_data\n");
+	cd_dbg(cdi, "entering open_for_data\n");
 	ret = open_for_common(cdi, &tracks);
 	if (ret)
 		goto clean_up_and_return;
@@ -1171,34 +1149,33 @@ static int open_for_data(struct cdrom_device_info *cdi)
 		if (cdi->options & CDO_CHECK_TYPE) {
 		    /* give people a warning shot, now that CDO_CHECK_TYPE
 		       is the default case! */
-		    cd_dbg(CD_OPEN, "bummer. wrong media type.\n");
-		    cd_dbg(CD_WARNING, "pid %d must open device O_NONBLOCK!\n",
+		    cd_dbg(cdi, "bummer. wrong media type.\n");
+		    cd_dbg(cdi, "pid %d must open device O_NONBLOCK!\n",
 			   (unsigned int)task_pid_nr(current));
 		    ret=-EMEDIUMTYPE;
 		    goto clean_up_and_return;
-		}
-		else {
-		    cd_dbg(CD_OPEN, "wrong media type, but CDO_CHECK_TYPE not set\n");
+		} else {
+			cd_dbg(cdi, "wrong media type, but CDO_CHECK_TYPE not set\n");
 		}
 	}
 
-	cd_dbg(CD_OPEN, "all seems well, opening the devicen");
+	cd_dbg(cdi, "all seems well, opening the devicen");
 
 	/* all seems well, we can open the device */
 	ret = cdo->open(cdi, 0); /* open for data */
-	cd_dbg(CD_OPEN, "opening the device gave me %d\n", ret);
+	cd_dbg(cdi, "opening the device gave me %d\n", ret);
 	/* After all this careful checking, we shouldn't have problems
 	   opening the device, but we don't want the device locked if 
 	   this somehow fails... */
 	if (ret) {
-		cd_dbg(CD_OPEN, "open device failed\n");
+		cd_dbg(cdi, "open device failed\n");
 		goto clean_up_and_return;
 	}
 	if (CDROM_CAN(CDC_LOCK) && (cdi->options & CDO_LOCK)) {
 			cdo->lock_door(cdi, 1);
-			cd_dbg(CD_OPEN, "door locked\n");
+			cd_dbg(cdi, "door locked\n");
 	}
-	cd_dbg(CD_OPEN, "device opened successfully\n");
+	cd_dbg(cdi, "device opened successfully\n");
 	return ret;
 
 	/* Something failed.  Try to unlock the drive, because some drivers
@@ -1207,10 +1184,10 @@ static int open_for_data(struct cdrom_device_info *cdi)
 	This ensures that the drive gets unlocked after a mount fails.  This 
 	is a goto to avoid bloating the driver with redundant code. */ 
 clean_up_and_return:
-	cd_dbg(CD_OPEN, "open failed\n");
+	cd_dbg(cdi, "open failed\n");
 	if (CDROM_CAN(CDC_LOCK) && cdi->options & CDO_LOCK) {
 			cdo->lock_door(cdi, 0);
-			cd_dbg(CD_OPEN, "door unlocked\n");
+			cd_dbg(cdi, "door unlocked\n");
 	}
 	return ret;
 }
@@ -1228,7 +1205,7 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
 {
 	int ret;
 
-	cd_dbg(CD_OPEN, "entering cdrom_open\n");
+	cd_dbg(cdi, "entering cdrom_open\n");
 
 	/* if this was a O_NONBLOCK open and we should honor the flags,
 	 * do a quick open without drive/disc integrity checks. */
@@ -1254,13 +1231,12 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
 	if (ret)
 		goto err;
 
-	cd_dbg(CD_OPEN, "Use count for \"/dev/%s\" now %d\n",
-	       cdi->name, cdi->use_count);
+	cd_dbg(cdi, "Use count now %d\n", cdi->use_count);
 	return 0;
 err_release:
 	if (CDROM_CAN(CDC_LOCK) && cdi->options & CDO_LOCK) {
 		cdi->ops->lock_door(cdi, 0);
-		cd_dbg(CD_OPEN, "door unlocked\n");
+		cd_dbg(cdi, "door unlocked\n");
 	}
 	cdi->ops->release(cdi);
 err:
@@ -1275,7 +1251,7 @@ static int check_for_audio_disc(struct cdrom_device_info *cdi)
 {
         int ret;
 	tracktype tracks;
-	cd_dbg(CD_OPEN, "entering check_for_audio_disc\n");
+	cd_dbg(cdi, "entering check_for_audio_disc\n");
 	if (!(cdi->options & CDO_CHECK_TYPE))
 		return 0;
 
@@ -1294,18 +1270,17 @@ void cdrom_release(struct cdrom_device_info *cdi, fmode_t mode)
 	const struct cdrom_device_ops *cdo = cdi->ops;
 	int opened_for_data;
 
-	cd_dbg(CD_CLOSE, "entering cdrom_release\n");
+	cd_dbg(cdi, "entering cdrom_release\n");
 
 	if (cdi->use_count > 0)
 		cdi->use_count--;
 
 	if (cdi->use_count == 0) {
-		cd_dbg(CD_CLOSE, "Use count for \"/dev/%s\" now zero\n",
-		       cdi->name);
+		cd_dbg(cdi, "Use count now zero\n");
 		cdrom_dvd_rw_close_write(cdi);
 
 		if ((cdo->capability & CDC_LOCK) && !cdi->keeplocked) {
-			cd_dbg(CD_CLOSE, "Unlocking door!\n");
+			cd_dbg(cdi, "Unlocking door!\n");
 			cdo->lock_door(cdi, 0);
 		}
 	}
@@ -1364,7 +1339,7 @@ static int cdrom_slot_status(struct cdrom_device_info *cdi, int slot)
 	struct cdrom_changer_info *info;
 	int ret;
 
-	cd_dbg(CD_CHANGER, "entering cdrom_slot_status()\n");
+	cd_dbg(cdi, "entering cdrom_slot_status()\n");
 	if (cdi->sanyo_slot)
 		return CDS_NO_INFO;
 	
@@ -1394,7 +1369,7 @@ int cdrom_number_of_slots(struct cdrom_device_info *cdi)
 	int nslots = 1;
 	struct cdrom_changer_info *info;
 
-	cd_dbg(CD_CHANGER, "entering cdrom_number_of_slots()\n");
+	cd_dbg(cdi, "entering cdrom_number_of_slots()\n");
 	/* cdrom_read_mech_status requires a valid value for capacity: */
 	cdi->capacity = 0; 
 
@@ -1415,7 +1390,7 @@ static int cdrom_load_unload(struct cdrom_device_info *cdi, int slot)
 {
 	struct packet_command cgc;
 
-	cd_dbg(CD_CHANGER, "entering cdrom_load_unload()\n");
+	cd_dbg(cdi, "entering cdrom_load_unload()\n");
 	if (cdi->sanyo_slot && slot < 0)
 		return 0;
 
@@ -1444,7 +1419,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
 	int curslot;
 	int ret;
 
-	cd_dbg(CD_CHANGER, "entering cdrom_select_disc()\n");
+	cd_dbg(cdi, "entering cdrom_select_disc()\n");
 	if (!CDROM_CAN(CDC_SELECT_DISC))
 		return -EDRIVE_CANT_DO_THIS;
 
@@ -1689,7 +1664,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 	switch (ai->type) {
 	/* LU data send */
 	case DVD_LU_SEND_AGID:
-		cd_dbg(CD_DVD, "entering DVD_LU_SEND_AGID\n");
+		cd_dbg(cdi, "entering DVD_LU_SEND_AGID\n");
 		cgc.quiet = 1;
 		setup_report_key(&cgc, ai->lsa.agid, 0);
 
@@ -1701,7 +1676,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		break;
 
 	case DVD_LU_SEND_KEY1:
-		cd_dbg(CD_DVD, "entering DVD_LU_SEND_KEY1\n");
+		cd_dbg(cdi, "entering DVD_LU_SEND_KEY1\n");
 		setup_report_key(&cgc, ai->lsk.agid, 2);
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
@@ -1712,7 +1687,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		break;
 
 	case DVD_LU_SEND_CHALLENGE:
-		cd_dbg(CD_DVD, "entering DVD_LU_SEND_CHALLENGE\n");
+		cd_dbg(cdi, "entering DVD_LU_SEND_CHALLENGE\n");
 		setup_report_key(&cgc, ai->lsc.agid, 1);
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
@@ -1724,7 +1699,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 
 	/* Post-auth key */
 	case DVD_LU_SEND_TITLE_KEY:
-		cd_dbg(CD_DVD, "entering DVD_LU_SEND_TITLE_KEY\n");
+		cd_dbg(cdi, "entering DVD_LU_SEND_TITLE_KEY\n");
 		cgc.quiet = 1;
 		setup_report_key(&cgc, ai->lstk.agid, 4);
 		cgc.cmd[5] = ai->lstk.lba;
@@ -1743,7 +1718,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		break;
 
 	case DVD_LU_SEND_ASF:
-		cd_dbg(CD_DVD, "entering DVD_LU_SEND_ASF\n");
+		cd_dbg(cdi, "entering DVD_LU_SEND_ASF\n");
 		setup_report_key(&cgc, ai->lsasf.agid, 5);
 		
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
@@ -1754,7 +1729,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 
 	/* LU data receive (LU changes state) */
 	case DVD_HOST_SEND_CHALLENGE:
-		cd_dbg(CD_DVD, "entering DVD_HOST_SEND_CHALLENGE\n");
+		cd_dbg(cdi, "entering DVD_HOST_SEND_CHALLENGE\n");
 		setup_send_key(&cgc, ai->hsc.agid, 1);
 		buf[1] = 0xe;
 		copy_chal(&buf[4], ai->hsc.chal);
@@ -1766,7 +1741,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		break;
 
 	case DVD_HOST_SEND_KEY2:
-		cd_dbg(CD_DVD, "entering DVD_HOST_SEND_KEY2\n");
+		cd_dbg(cdi, "entering DVD_HOST_SEND_KEY2\n");
 		setup_send_key(&cgc, ai->hsk.agid, 3);
 		buf[1] = 0xa;
 		copy_key(&buf[4], ai->hsk.key);
@@ -1781,7 +1756,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 	/* Misc */
 	case DVD_INVALIDATE_AGID:
 		cgc.quiet = 1;
-		cd_dbg(CD_DVD, "entering DVD_INVALIDATE_AGID\n");
+		cd_dbg(cdi, "entering DVD_INVALIDATE_AGID\n");
 		setup_report_key(&cgc, ai->lsa.agid, 0x3f);
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
 			return ret;
@@ -1789,7 +1764,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 
 	/* Get region settings */
 	case DVD_LU_SEND_RPC_STATE:
-		cd_dbg(CD_DVD, "entering DVD_LU_SEND_RPC_STATE\n");
+		cd_dbg(cdi, "entering DVD_LU_SEND_RPC_STATE\n");
 		setup_report_key(&cgc, 0, 8);
 		memset(&rpc_state, 0, sizeof(rpc_state_t));
 		cgc.buffer = (char *) &rpc_state;
@@ -1806,7 +1781,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 
 	/* Set region settings */
 	case DVD_HOST_SEND_RPC_STATE:
-		cd_dbg(CD_DVD, "entering DVD_HOST_SEND_RPC_STATE\n");
+		cd_dbg(cdi, "entering DVD_HOST_SEND_RPC_STATE\n");
 		setup_send_key(&cgc, 0, 6);
 		buf[1] = 6;
 		buf[4] = ai->hrpcs.pdrc;
@@ -1816,7 +1791,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		break;
 
 	default:
-		cd_dbg(CD_WARNING, "Invalid DVD key ioctl (%d)\n", ai->type);
+		cd_dbg(cdi, "Invalid DVD key ioctl (%d)\n", ai->type);
 		return -ENOTTY;
 	}
 
@@ -1948,8 +1923,7 @@ static int dvd_read_bca(struct cdrom_device_info *cdi, dvd_struct *s,
 
 	s->bca.len = buf[0] << 8 | buf[1];
 	if (s->bca.len < 12 || s->bca.len > 188) {
-		cd_dbg(CD_WARNING, "Received invalid BCA length (%d)\n",
-		       s->bca.len);
+		cd_dbg(cdi, "Received invalid BCA length (%d)\n", s->bca.len);
 		ret = -EIO;
 		goto out;
 	}
@@ -1985,13 +1959,13 @@ static int dvd_read_manufact(struct cdrom_device_info *cdi, dvd_struct *s,
 
 	s->manufact.len = buf[0] << 8 | buf[1];
 	if (s->manufact.len < 0) {
-		cd_dbg(CD_WARNING, "Received invalid manufacture info length (%d)\n",
-		       s->manufact.len);
+		cd_dbg(cdi, "Received invalid manufacture info length (%d)\n",
+			s->manufact.len);
 		ret = -EIO;
 	} else {
 		if (s->manufact.len > 2048) {
-			cd_dbg(CD_WARNING, "Received invalid manufacture info length (%d): truncating to 2048\n",
-			       s->manufact.len);
+			cd_dbg(cdi, "Received invalid manufacture info length (%d): truncating to 2048\n",
+				s->manufact.len);
 			s->manufact.len = 2048;
 		}
 		memcpy(s->manufact.value, &buf[4], s->manufact.len);
@@ -2022,8 +1996,8 @@ static int dvd_read_struct(struct cdrom_device_info *cdi, dvd_struct *s,
 		return dvd_read_manufact(cdi, s, cgc);
 		
 	default:
-		cd_dbg(CD_WARNING, ": Invalid DVD structure read requested (%d)\n",
-		       s->type);
+		cd_dbg(cdi, "Invalid DVD structure read requested (%d)\n",
+			s->type);
 		return -EINVAL;
 	}
 }
@@ -2308,7 +2282,7 @@ static int cdrom_read_cdda(struct cdrom_device_info *cdi, __u8 __user *ubuf,
 	 * frame dma, so drop to single frame dma if we need to
 	 */
 	if (cdi->cdda_method == CDDA_BPC_FULL && nframes > 1) {
-		pr_info("dropping to single frame dma\n");
+		cd_info(cdi, "dropping to single frame dma\n");
 		cdi->cdda_method = CDDA_BPC_SINGLE;
 		goto retry;
 	}
@@ -2321,7 +2295,8 @@ static int cdrom_read_cdda(struct cdrom_device_info *cdi, __u8 __user *ubuf,
 	if (cdi->last_sense != 0x04 && cdi->last_sense != 0x0b)
 		return ret;
 
-	pr_info("dropping to old style cdda (sense=%x)\n", cdi->last_sense);
+	cd_info(cdi, "dropping to old style cdda (sense=%x)\n",
+		cdi->last_sense);
 	cdi->cdda_method = CDDA_OLD;
 	return cdrom_read_cdda_old(cdi, ubuf, lba, nframes);	
 }
@@ -2333,7 +2308,7 @@ static int cdrom_ioctl_multisession(struct cdrom_device_info *cdi,
 	u8 requested_format;
 	int ret;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROMMULTISESSION\n");
+	cd_dbg(cdi, "entering CDROMMULTISESSION\n");
 
 	if (!(cdi->ops->capability & CDC_MULTI_SESSION))
 		return -ENOSYS;
@@ -2355,13 +2330,13 @@ static int cdrom_ioctl_multisession(struct cdrom_device_info *cdi,
 	if (copy_to_user(argp, &ms_info, sizeof(ms_info)))
 		return -EFAULT;
 
-	cd_dbg(CD_DO_IOCTL, "CDROMMULTISESSION successful\n");
+	cd_dbg(cdi, "CDROMMULTISESSION successful\n");
 	return 0;
 }
 
 static int cdrom_ioctl_eject(struct cdrom_device_info *cdi)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROMEJECT\n");
+	cd_dbg(cdi, "entering CDROMEJECT\n");
 
 	if (!CDROM_CAN(CDC_OPEN_TRAY))
 		return -ENOSYS;
@@ -2378,7 +2353,7 @@ static int cdrom_ioctl_eject(struct cdrom_device_info *cdi)
 
 static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROMCLOSETRAY\n");
+	cd_dbg(cdi, "entering CDROMCLOSETRAY\n");
 
 	if (!CDROM_CAN(CDC_CLOSE_TRAY))
 		return -ENOSYS;
@@ -2388,7 +2363,7 @@ static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi)
 static int cdrom_ioctl_eject_sw(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROMEJECT_SW\n");
+	cd_dbg(cdi, "entering CDROMEJECT_SW\n");
 
 	if (!CDROM_CAN(CDC_OPEN_TRAY))
 		return -ENOSYS;
@@ -2407,7 +2382,7 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
 	struct cdrom_changer_info *info;
 	int ret;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_MEDIA_CHANGED\n");
+	cd_dbg(cdi, "entering CDROM_MEDIA_CHANGED\n");
 
 	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
 		return -ENOSYS;
@@ -2433,7 +2408,7 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
 static int cdrom_ioctl_set_options(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_SET_OPTIONS\n");
+	cd_dbg(cdi, "entering CDROM_SET_OPTIONS\n");
 
 	/*
 	 * Options need to be in sync with capability.
@@ -2461,7 +2436,7 @@ static int cdrom_ioctl_set_options(struct cdrom_device_info *cdi,
 static int cdrom_ioctl_clear_options(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_CLEAR_OPTIONS\n");
+	cd_dbg(cdi, "entering CDROM_CLEAR_OPTIONS\n");
 
 	cdi->options &= ~(int) arg;
 	return cdi->options;
@@ -2470,7 +2445,7 @@ static int cdrom_ioctl_clear_options(struct cdrom_device_info *cdi,
 static int cdrom_ioctl_select_speed(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_SELECT_SPEED\n");
+	cd_dbg(cdi, "entering CDROM_SELECT_SPEED\n");
 
 	if (!CDROM_CAN(CDC_SELECT_SPEED))
 		return -ENOSYS;
@@ -2480,7 +2455,7 @@ static int cdrom_ioctl_select_speed(struct cdrom_device_info *cdi,
 static int cdrom_ioctl_select_disc(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_SELECT_DISC\n");
+	cd_dbg(cdi, "entering CDROM_SELECT_DISC\n");
 
 	if (!CDROM_CAN(CDC_SELECT_DISC))
 		return -ENOSYS;
@@ -2498,14 +2473,14 @@ static int cdrom_ioctl_select_disc(struct cdrom_device_info *cdi,
 	if (cdi->ops->select_disc)
 		return cdi->ops->select_disc(cdi, arg);
 
-	cd_dbg(CD_CHANGER, "Using generic cdrom_select_disc()\n");
+	cd_dbg(cdi, "Using generic cdrom_select_disc()\n");
 	return cdrom_select_disc(cdi, arg);
 }
 
 static int cdrom_ioctl_reset(struct cdrom_device_info *cdi,
 		struct block_device *bdev)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_RESET\n");
+	cd_dbg(cdi, "entering CDROM_RESET\n");
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -2518,7 +2493,7 @@ static int cdrom_ioctl_reset(struct cdrom_device_info *cdi,
 static int cdrom_ioctl_lock_door(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "%socking door\n", arg ? "L" : "Unl");
+	cd_dbg(cdi, "%socking door\n", arg ? "L" : "Unl");
 
 	if (!CDROM_CAN(CDC_LOCK))
 		return -EDRIVE_CANT_DO_THIS;
@@ -2537,7 +2512,7 @@ static int cdrom_ioctl_lock_door(struct cdrom_device_info *cdi,
 static int cdrom_ioctl_debug(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "%sabling debug\n", arg ? "En" : "Dis");
+	cd_dbg(cdi, "%sabling debug\n", arg ? "En" : "Dis");
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -2547,7 +2522,7 @@ static int cdrom_ioctl_debug(struct cdrom_device_info *cdi,
 
 static int cdrom_ioctl_get_capability(struct cdrom_device_info *cdi)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_GET_CAPABILITY\n");
+	cd_dbg(cdi, "entering CDROM_GET_CAPABILITY\n");
 	return (cdi->ops->capability & ~cdi->mask);
 }
 
@@ -2563,7 +2538,7 @@ static int cdrom_ioctl_get_mcn(struct cdrom_device_info *cdi,
 	struct cdrom_mcn mcn;
 	int ret;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_GET_MCN\n");
+	cd_dbg(cdi, "entering CDROM_GET_MCN\n");
 
 	if (!(cdi->ops->capability & CDC_MCN))
 		return -ENOSYS;
@@ -2573,14 +2548,14 @@ static int cdrom_ioctl_get_mcn(struct cdrom_device_info *cdi,
 
 	if (copy_to_user(argp, &mcn, sizeof(mcn)))
 		return -EFAULT;
-	cd_dbg(CD_DO_IOCTL, "CDROM_GET_MCN successful\n");
+	cd_dbg(cdi, "CDROM_GET_MCN successful\n");
 	return 0;
 }
 
 static int cdrom_ioctl_drive_status(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_DRIVE_STATUS\n");
+	cd_dbg(cdi, "entering CDROM_DRIVE_STATUS\n");
 
 	if (!(cdi->ops->capability & CDC_DRIVE_STATUS))
 		return -ENOSYS;
@@ -2613,7 +2588,7 @@ static int cdrom_ioctl_disc_status(struct cdrom_device_info *cdi)
 {
 	tracktype tracks;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_DISC_STATUS\n");
+	cd_dbg(cdi, "entering CDROM_DISC_STATUS\n");
 
 	cdrom_count_tracks(cdi, &tracks);
 	if (tracks.error)
@@ -2635,13 +2610,13 @@ static int cdrom_ioctl_disc_status(struct cdrom_device_info *cdi)
 		return CDS_DATA_1;
 	/* Policy mode off */
 
-	cd_dbg(CD_WARNING, "This disc doesn't have any tracks I recognize!\n");
+	cd_dbg(cdi, "This disc doesn't have any tracks I recognize!\n");
 	return CDS_NO_INFO;
 }
 
 static int cdrom_ioctl_changer_nslots(struct cdrom_device_info *cdi)
 {
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_CHANGER_NSLOTS\n");
+	cd_dbg(cdi, "entering CDROM_CHANGER_NSLOTS\n");
 	return cdi->capacity;
 }
 
@@ -2652,7 +2627,7 @@ static int cdrom_ioctl_get_subchnl(struct cdrom_device_info *cdi,
 	u8 requested, back;
 	int ret;
 
-	/* cd_dbg(CD_DO_IOCTL,"entering CDROMSUBCHNL\n");*/
+	cd_dbg(cdi, "entering CDROMSUBCHNL\n");
 
 	if (copy_from_user(&q, argp, sizeof(q)))
 		return -EFAULT;
@@ -2672,7 +2647,7 @@ static int cdrom_ioctl_get_subchnl(struct cdrom_device_info *cdi,
 
 	if (copy_to_user(argp, &q, sizeof(q)))
 		return -EFAULT;
-	/* cd_dbg(CD_DO_IOCTL, "CDROMSUBCHNL successful\n"); */
+	cd_dbg(cdi, "CDROMSUBCHNL successful\n");
 	return 0;
 }
 
@@ -2682,7 +2657,7 @@ static int cdrom_ioctl_read_tochdr(struct cdrom_device_info *cdi,
 	struct cdrom_tochdr header;
 	int ret;
 
-	/* cd_dbg(CD_DO_IOCTL, "entering CDROMREADTOCHDR\n"); */
+	cd_dbg(cdi, "entering CDROMREADTOCHDR\n");
 
 	if (copy_from_user(&header, argp, sizeof(header)))
 		return -EFAULT;
@@ -2693,7 +2668,7 @@ static int cdrom_ioctl_read_tochdr(struct cdrom_device_info *cdi,
 
 	if (copy_to_user(argp, &header, sizeof(header)))
 		return -EFAULT;
-	/* cd_dbg(CD_DO_IOCTL, "CDROMREADTOCHDR successful\n"); */
+	cd_dbg(cdi, "CDROMREADTOCHDR successful\n");
 	return 0;
 }
 
@@ -2704,7 +2679,7 @@ static int cdrom_ioctl_read_tocentry(struct cdrom_device_info *cdi,
 	u8 requested_format;
 	int ret;
 
-	/* cd_dbg(CD_DO_IOCTL, "entering CDROMREADTOCENTRY\n"); */
+	cd_dbg(cdi, "entering CDROMREADTOCENTRY\n");
 
 	if (copy_from_user(&entry, argp, sizeof(entry)))
 		return -EFAULT;
@@ -2721,7 +2696,7 @@ static int cdrom_ioctl_read_tocentry(struct cdrom_device_info *cdi,
 
 	if (copy_to_user(argp, &entry, sizeof(entry)))
 		return -EFAULT;
-	/* cd_dbg(CD_DO_IOCTL, "CDROMREADTOCENTRY successful\n"); */
+	cd_dbg(cdi, "CDROMREADTOCENTRY successful\n");
 	return 0;
 }
 
@@ -2730,7 +2705,7 @@ static int cdrom_ioctl_play_msf(struct cdrom_device_info *cdi,
 {
 	struct cdrom_msf msf;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROMPLAYMSF\n");
+	cd_dbg(cdi, "entering CDROMPLAYMSF\n");
 
 	if (!CDROM_CAN(CDC_PLAY_AUDIO))
 		return -ENOSYS;
@@ -2745,7 +2720,7 @@ static int cdrom_ioctl_play_trkind(struct cdrom_device_info *cdi,
 	struct cdrom_ti ti;
 	int ret;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROMPLAYTRKIND\n");
+	cd_dbg(cdi, "entering CDROMPLAYTRKIND\n");
 
 	if (!CDROM_CAN(CDC_PLAY_AUDIO))
 		return -ENOSYS;
@@ -2762,7 +2737,7 @@ static int cdrom_ioctl_volctrl(struct cdrom_device_info *cdi,
 {
 	struct cdrom_volctrl volume;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROMVOLCTRL\n");
+	cd_dbg(cdi, "entering CDROMVOLCTRL\n");
 
 	if (!CDROM_CAN(CDC_PLAY_AUDIO))
 		return -ENOSYS;
@@ -2777,7 +2752,7 @@ static int cdrom_ioctl_volread(struct cdrom_device_info *cdi,
 	struct cdrom_volctrl volume;
 	int ret;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROMVOLREAD\n");
+	cd_dbg(cdi, "entering CDROMVOLREAD\n");
 
 	if (!CDROM_CAN(CDC_PLAY_AUDIO))
 		return -ENOSYS;
@@ -2796,7 +2771,7 @@ static int cdrom_ioctl_audioctl(struct cdrom_device_info *cdi,
 {
 	int ret;
 
-	cd_dbg(CD_DO_IOCTL, "doing audio ioctl (start/stop/pause/resume)\n");
+	cd_dbg(cdi, "doing audio ioctl (start/stop/pause/resume)\n");
 
 	if (!CDROM_CAN(CDC_PLAY_AUDIO))
 		return -ENOSYS;
@@ -3089,7 +3064,7 @@ static noinline int mmc_ioctl_cdrom_subchannel(struct cdrom_device_info *cdi,
 	sanitize_format(&q.cdsc_reladdr, &q.cdsc_format, requested);
 	if (copy_to_user((struct cdrom_subchnl __user *)arg, &q, sizeof(q)))
 		return -EFAULT;
-	/* cd_dbg(CD_DO_IOCTL, "CDROMSUBCHNL successful\n"); */
+	cd_dbg(cdi, "CDROMSUBCHNL successful\n");
 	return 0;
 }
 
@@ -3099,7 +3074,7 @@ static noinline int mmc_ioctl_cdrom_play_msf(struct cdrom_device_info *cdi,
 {
 	const struct cdrom_device_ops *cdo = cdi->ops;
 	struct cdrom_msf msf;
-	cd_dbg(CD_DO_IOCTL, "entering CDROMPLAYMSF\n");
+	cd_dbg(cdi, "entering CDROMPLAYMSF\n");
 	if (copy_from_user(&msf, (struct cdrom_msf __user *)arg, sizeof(msf)))
 		return -EFAULT;
 	cgc->cmd[0] = GPCMD_PLAY_AUDIO_MSF;
@@ -3119,7 +3094,7 @@ static noinline int mmc_ioctl_cdrom_play_blk(struct cdrom_device_info *cdi,
 {
 	const struct cdrom_device_ops *cdo = cdi->ops;
 	struct cdrom_blk blk;
-	cd_dbg(CD_DO_IOCTL, "entering CDROMPLAYBLK\n");
+	cd_dbg(cdi, "entering CDROMPLAYBLK\n");
 	if (copy_from_user(&blk, (struct cdrom_blk __user *)arg, sizeof(blk)))
 		return -EFAULT;
 	cgc->cmd[0] = GPCMD_PLAY_AUDIO_10;
@@ -3144,7 +3119,7 @@ static noinline int mmc_ioctl_cdrom_volume(struct cdrom_device_info *cdi,
 	unsigned short offset;
 	int ret;
 
-	cd_dbg(CD_DO_IOCTL, "entering CDROMVOLUME\n");
+	cd_dbg(cdi, "entering CDROMVOLUME\n");
 
 	if (copy_from_user(&volctrl, (struct cdrom_volctrl __user *)arg,
 			   sizeof(volctrl)))
@@ -3213,7 +3188,7 @@ static noinline int mmc_ioctl_cdrom_start_stop(struct cdrom_device_info *cdi,
 					       int cmd)
 {
 	const struct cdrom_device_ops *cdo = cdi->ops;
-	cd_dbg(CD_DO_IOCTL, "entering CDROMSTART/CDROMSTOP\n");
+	cd_dbg(cdi, "entering CDROMSTART/CDROMSTOP\n");
 	cgc->cmd[0] = GPCMD_START_STOP_UNIT;
 	cgc->cmd[1] = 1;
 	cgc->cmd[4] = (cmd == CDROMSTART) ? 1 : 0;
@@ -3226,7 +3201,7 @@ static noinline int mmc_ioctl_cdrom_pause_resume(struct cdrom_device_info *cdi,
 						 int cmd)
 {
 	const struct cdrom_device_ops *cdo = cdi->ops;
-	cd_dbg(CD_DO_IOCTL, "entering CDROMPAUSE/CDROMRESUME\n");
+	cd_dbg(cdi, "entering CDROMPAUSE/CDROMRESUME\n");
 	cgc->cmd[0] = GPCMD_PAUSE_RESUME;
 	cgc->cmd[8] = (cmd == CDROMRESUME) ? 1 : 0;
 	cgc->data_direction = CGC_DATA_NONE;
@@ -3248,7 +3223,7 @@ static noinline int mmc_ioctl_dvd_read_struct(struct cdrom_device_info *cdi,
 	if (IS_ERR(s))
 		return PTR_ERR(s);
 
-	cd_dbg(CD_DO_IOCTL, "entering DVD_READ_STRUCT\n");
+	cd_dbg(cdi, "entering DVD_READ_STRUCT\n");
 
 	ret = dvd_read_struct(cdi, s, cgc);
 	if (ret)
@@ -3268,7 +3243,7 @@ static noinline int mmc_ioctl_dvd_auth(struct cdrom_device_info *cdi,
 	dvd_authinfo ai;
 	if (!CDROM_CAN(CDC_DVD))
 		return -ENOSYS;
-	cd_dbg(CD_DO_IOCTL, "entering DVD_AUTH\n");
+	cd_dbg(cdi, "entering DVD_AUTH\n");
 	if (copy_from_user(&ai, (dvd_authinfo __user *)arg, sizeof(ai)))
 		return -EFAULT;
 	ret = dvd_do_auth(cdi, &ai);
@@ -3284,7 +3259,7 @@ static noinline int mmc_ioctl_cdrom_next_writable(struct cdrom_device_info *cdi,
 {
 	int ret;
 	long next = 0;
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_NEXT_WRITABLE\n");
+	cd_dbg(cdi, "entering CDROM_NEXT_WRITABLE\n");
 	ret = cdrom_get_next_writable(cdi, &next);
 	if (ret)
 		return ret;
@@ -3298,7 +3273,7 @@ static noinline int mmc_ioctl_cdrom_last_written(struct cdrom_device_info *cdi,
 {
 	int ret;
 	long last = 0;
-	cd_dbg(CD_DO_IOCTL, "entering CDROM_LAST_WRITTEN\n");
+	cd_dbg(cdi, "entering CDROM_LAST_WRITTEN\n");
 	ret = cdrom_get_last_written(cdi, &last);
 	if (ret)
 		return ret;
@@ -3419,11 +3394,6 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
 			return ret;
 	}
 
-	/*
-	 * Note: most of the cd_dbg() calls are commented out here,
-	 * because they fill up the sys log when CD players poll
-	 * the drive.
-	 */
 	switch (cmd) {
 	case CDROMSUBCHNL:
 		return cdrom_ioctl_get_subchnl(cdi, argp);
-- 
2.23.0

[PATCH v4 rebase 06/10] bdev: reset first_open when looping in __blkget_dev

[Michal Suchanek]

It is not clear that no other thread cannot open the block device when
__blkget_dev drops it and loop to restart label. Reset first_open to
false when looping.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 fs/block_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index ee63c2732fa2..545bb6c8848a 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1560,7 +1560,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 	int ret;
 	int partno;
 	int perm = 0;
-	bool first_open = false;
+	bool first_open;
 
 	if (mode & FMODE_READ)
 		perm |= MAY_READ;
@@ -1580,6 +1580,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
  restart:
 
 	ret = -ENXIO;
+	first_open = false;
 	disk = bdev_get_gendisk(bdev, &partno);
 	if (!disk)
 		goto out;
-- 
2.23.0

[PATCH v4 rebase 07/10] bdev: separate parts of __blkdev_get as helper functions

[Michal Suchanek]

This code will be reused in later patch. Hopefully putting it aside
rather than cut&pasting another copy will make the function more
readable.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 fs/block_dev.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 545bb6c8848a..a386ebd997fb 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1547,6 +1547,24 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
  */
 EXPORT_SYMBOL_GPL(bdev_disk_changed);
 
+static void blkdev_init_size(struct block_device *bdev)
+{
+	bd_set_size(bdev, (loff_t)get_capacity(bdev->bd_disk) << 9);
+	set_init_blocksize(bdev);
+}
+
+static void blkdev_do_partitions(struct block_device *bdev, int ret)
+{
+	/*
+	 * If the device is invalidated, rescan partition if open succeeded or
+	 * failed with -ENOMEDIUM.  The latter is necessary to prevent ghost
+	 * partitions on a removed medium.
+	 */
+	if (bdev->bd_invalidated &&
+			(!ret || ret == -ENOMEDIUM))
+		bdev_disk_changed(bdev, ret == -ENOMEDIUM);
+}
+
 /*
  * bd_mutex locking:
  *
@@ -1619,20 +1637,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 				}
 			}
 
-			if (!ret) {
-				bd_set_size(bdev,(loff_t)get_capacity(disk)<<9);
-				set_init_blocksize(bdev);
-			}
-
-			/*
-			 * If the device is invalidated, rescan partition
-			 * if open succeeded or failed with -ENOMEDIUM.
-			 * The latter is necessary to prevent ghost
-			 * partitions on a removed medium.
-			 */
-			if (bdev->bd_invalidated &&
-			    (!ret || ret == -ENOMEDIUM))
-				bdev_disk_changed(bdev, ret == -ENOMEDIUM);
+			if (!ret)
+				blkdev_init_size(bdev);
+			blkdev_do_partitions(bdev, ret);
 
 			if (ret)
 				goto out_clear;
@@ -1664,10 +1671,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 			ret = 0;
 			if (bdev->bd_disk->fops->open)
 				ret = bdev->bd_disk->fops->open(bdev, mode);
-			/* the same as first opener case, read comment there */
-			if (bdev->bd_invalidated &&
-			    (!ret || ret == -ENOMEDIUM))
-				bdev_disk_changed(bdev, ret == -ENOMEDIUM);
+
+			blkdev_do_partitions(bdev, ret);
+
 			if (ret)
 				goto out_unlock_bdev;
 		}
-- 
2.23.0

[PATCH v4 rebase 08/10] bdev: add open_finish

[Michal Suchanek]

Opening a block device may require a long operation such as waiting for
the cdrom tray to close or disk to spin up. Performing this operation
with locks held locks out other attempts to open the device from
processes that donn't care about the medium state.

To avoid this issue and still be able to perform time-consuming checks
at open() the block device driver can provide open_finish(). If it does
opening the device proceeds even when an error is returned from open(),
bd_mutex is released and open_finish() is called. If open_finish()
succeeds the device is now open, if it fails release() is called.

When -ERESTARTSYS is returned from open() blkdev_get may loop without
calling open_finish(). On -ERESTARTSYS open_finish() is not called.

When -ENXIO is returned there is no device to retry opening so this
error needs to be honored and open_finish() not called.

Move a ret = 0 assignment up in the if/else branching to avoid returning
-ENXIO. Previously the return value was ignored on the unhandled branch.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: new patch
v4:
 - fix crash on ENXIO
 - initialize capacity and partitions after open_finish
---
 Documentation/filesystems/locking.rst |  2 ++
 fs/block_dev.c                        | 27 +++++++++++++++++++++++----
 include/linux/blkdev.h                |  1 +
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index fc3a0704553c..2471ced5a8cf 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -456,6 +456,7 @@ block_device_operations
 prototypes::
 
 	int (*open) (struct block_device *, fmode_t);
+	int (*open_finish) (struct block_device *, fmode_t, int);
 	int (*release) (struct gendisk *, fmode_t);
 	int (*ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
 	int (*compat_ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
@@ -473,6 +474,7 @@ locking rules:
 ops			bd_mutex
 ======================= ===================
 open:			yes
+open_finish:		no
 release:		yes
 ioctl:			no
 compat_ioctl:		no
diff --git a/fs/block_dev.c b/fs/block_dev.c
index a386ebd997fb..787b204467f9 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1579,6 +1579,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 	int partno;
 	int perm = 0;
 	bool first_open;
+	bool need_finish = false;
 
 	if (mode & FMODE_READ)
 		perm |= MAY_READ;
@@ -1635,13 +1636,16 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 					put_disk_and_module(disk);
 					goto restart;
 				}
+				if ((ret != -ENXIO) &&
+				    bdev->bd_disk->fops->open_finish)
+					need_finish = true;
 			}
 
 			if (!ret)
 				blkdev_init_size(bdev);
 			blkdev_do_partitions(bdev, ret);
 
-			if (ret)
+			if (ret && !need_finish)
 				goto out_clear;
 		} else {
 			struct block_device *whole;
@@ -1667,14 +1671,18 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 		if (bdev->bd_bdi == &noop_backing_dev_info)
 			bdev->bd_bdi = bdi_get(disk->queue->backing_dev_info);
 	} else {
+		ret = 0;
 		if (bdev->bd_contains == bdev) {
-			ret = 0;
-			if (bdev->bd_disk->fops->open)
+			if (bdev->bd_disk->fops->open) {
 				ret = bdev->bd_disk->fops->open(bdev, mode);
 
+				if ((ret != -ERESTARTSYS) && (ret != -ENXIO) &&
+				    bdev->bd_disk->fops->open_finish)
+					need_finish = true;
+			}
 			blkdev_do_partitions(bdev, ret);
 
-			if (ret)
+			if (ret && !need_finish)
 				goto out_unlock_bdev;
 		}
 	}
@@ -1686,6 +1694,17 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 	/* only one opener holds refs to the module and disk */
 	if (!first_open)
 		put_disk_and_module(disk);
+	if (ret && need_finish) {
+		ret = bdev->bd_disk->fops->open_finish(bdev, mode, ret);
+
+		if (!ret && first_open)
+			blkdev_init_size(bdev);
+		blkdev_do_partitions(bdev, ret);
+	}
+	if (ret) {
+		__blkdev_put(bdev, mode, for_part);
+		return ret;
+	}
 	return 0;
 
  out_clear:
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 397bb9bc230b..d26264f5da39 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1694,6 +1694,7 @@ static inline struct bio_vec *rq_integrity_vec(struct request *rq)
 
 struct block_device_operations {
 	int (*open) (struct block_device *, fmode_t);
+	int (*open_finish)(struct block_device *bdev, fmode_t mode, int ret);
 	void (*release) (struct gendisk *, fmode_t);
 	int (*rw_page)(struct block_device *, sector_t, struct page *, unsigned int);
 	int (*ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
-- 
2.23.0

[PATCH v4 rebase 09/10] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[Michal Suchanek]

The WMware ESXi cdrom identifies itself as:
sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
model: "VMware SATA CD001.00"
with the following get_capabilities print in sr.c:
        sr_printk(KERN_INFO, cd,
                  "scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
                  cd->device->vendor, cd->device->model);

The model looks like reliable identification while vendor does not.

The drive claims to have a tray and claims to be able to close it.
However, the UI has no notion of a tray - when medium is ejected it is
dropped in the floor and the user must select a medium again before the
drive can be re-loaded.  On the kernel side the tray_move call to close
the tray succeeds but the drive state does not change as a result of the
call.

The drive does not in fact emulate the tray state. There are two ways to
get the medium state. One is the SCSI status:

Physical drive:

Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray open
Raw sense data (in hex):
        70 00 02 00 00 00 00 0a  00 00 00 00 3a 02 00 00
        00 00

Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray closed
 Raw sense data (in hex):
        70 00 02 00 00 00 00 0a  00 00 00 00 3a 01 00 00
        00 00

VMware ESXi:

Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present
  Info fld=0x0 [0]
 Raw sense data (in hex):
        f0 00 02 00 00 00 00 0a  00 00 00 00 3a 00 00 00
        00 00

So the tray state is not reported here. Other is medium status which the
kernel prefers if available. Adding a print here gives:

cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0

door_open is interpreted as open tray. This is fine so long as tray_move
would close the tray when requested or report an error which never
happens on VMware ESXi servers (5.5 and 6.5 tested).

This is a popular virtualization platform so a workaround is worthwhile.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: new patch
v3: change into a blacklist flag
v4: fix vendor match condition
---
 drivers/scsi/scsi_devinfo.c | 15 +++++++++------
 drivers/scsi/sr.c           |  6 ++++++
 include/scsi/scsi_devinfo.h |  7 ++++++-
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index df14597752ec..923f54b88d24 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -252,6 +252,7 @@ static struct {
 	{"TOSHIBA", "CD-ROM", NULL, BLIST_ISROM},
 	{"Traxdata", "CDR4120", NULL, BLIST_NOLUN},	/* locks up */
 	{"USB2.0", "SMARTMEDIA/XD", NULL, BLIST_FORCELUN | BLIST_INQUIRY_36},
+	{"VMware", "VMware", NULL, BLIST_NO_MATCH_VENDOR | BLIST_NO_TRAY},
 	{"WangDAT", "Model 2600", "01.7", BLIST_SELECT_NO_ATN},
 	{"WangDAT", "Model 3200", "02.2", BLIST_SELECT_NO_ATN},
 	{"WangDAT", "Model 1300", "02.4", BLIST_SELECT_NO_ATN},
@@ -454,10 +455,11 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 			/*
 			 * vendor strings must be an exact match
 			 */
-			if (vmax != strnlen(devinfo->vendor,
-					    sizeof(devinfo->vendor)) ||
-			    memcmp(devinfo->vendor, vskip, vmax))
-				continue;
+			if (!(devinfo->flags & BLIST_NO_MATCH_VENDOR))
+				if (vmax != strnlen(devinfo->vendor,
+						    sizeof(devinfo->vendor)) ||
+				    memcmp(devinfo->vendor, vskip, vmax))
+					continue;
 
 			/*
 			 * @model specifies the full string, and
@@ -468,8 +470,9 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 				continue;
 			return devinfo;
 		} else {
-			if (!memcmp(devinfo->vendor, vendor,
-				    sizeof(devinfo->vendor)) &&
+			if ((!memcmp(devinfo->vendor, vendor,
+				    sizeof(devinfo->vendor))
+			       || (devinfo->flags & BLIST_NO_MATCH_VENDOR)) &&
 			    !memcmp(devinfo->model, model,
 				    sizeof(devinfo->model)))
 				return devinfo;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4664fdf75c0f..07c319494bf4 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -58,6 +58,7 @@
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>	/* For the door lock/unlock commands */
+#include <scsi/scsi_devinfo.h>
 
 #include "scsi_logging.h"
 #include "sr.h"
@@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
 		  buffer[n + 4] & 0x20 ? "xa/form2 " : "",	/* can read xa/from2 */
 		  buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
 		  loadmech[buffer[n + 6] >> 5]);
+	if (cd->device->sdev_bflags & BLIST_NO_TRAY) {
+		buffer[n + 6] &= ~(0xff << 5);
+		sr_printk(KERN_INFO, cd,
+			  "Tray emulation bug workaround: tray -> caddy\n");
+	}
 	if ((buffer[n + 6] >> 5) == 0)
 		/* caddy drives can't close tray... */
 		cd->cdi.mask |= CDC_CLOSE_TRAY;
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 3fdb322d4c4b..17ea96936cc6 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -67,8 +67,13 @@
 #define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
 /* Always retry ABORTED_COMMAND with ASC 0xc1 */
 #define BLIST_RETRY_ASC_C1	((__force blist_flags_t)(1ULL << 33))
+/* Device reports to have a tray but it cannot be operated reliably */
+#define BLIST_NO_TRAY		((__force blist_flags_t)(1ULL << 34))
+/* Vendor string is bogus */
+#define BLIST_NO_MATCH_VENDOR	((__force blist_flags_t)(1ULL << 35))
 
-#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
+
+#define __BLIST_LAST_USED BLIST_NO_MATCH_VENDOR
 
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \
-- 
2.23.0

[PATCH v4 rebase 10/10] scsi: sr: wait for the medium to become ready

[Michal Suchanek]

Use the autoclose function provided by cdrom driver to wait for drive to
close in open_finish, and attempt to open once more after the door
closes.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v3: use function call rather than IOCTL
v4: fix reference leak on -ENXIO
---
 drivers/scsi/sr.c | 54 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 07c319494bf4..41ddf08b4c7b 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -522,29 +522,58 @@ static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt)
 	return ret;
 }
 
-static int sr_block_open(struct block_device *bdev, fmode_t mode)
+static int __sr_block_open(struct block_device *bdev, fmode_t mode)
 {
-	struct scsi_cd *cd;
-	struct scsi_device *sdev;
-	int ret = -ENXIO;
-
-	cd = scsi_cd_get(bdev->bd_disk);
-	if (!cd)
-		goto out;
+	struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
+	int ret;
 
-	sdev = cd->device;
-	scsi_autopm_get_device(sdev);
 	check_disk_change(bdev);
 
 	mutex_lock(&sr_mutex);
 	ret = cdrom_open(&cd->cdi, bdev, mode);
 	mutex_unlock(&sr_mutex);
 
+	return ret;
+}
+
+static int sr_block_open(struct block_device *bdev, fmode_t mode)
+{
+	struct scsi_cd *cd = scsi_cd_get(bdev->bd_disk);
+	struct scsi_device *sdev;
+	int ret;
+
+	if (!cd)
+		return -ENXIO;
+
+	sdev = cd->device;
+	scsi_autopm_get_device(sdev);
+	ret = __sr_block_open(bdev, mode);
 	scsi_autopm_put_device(sdev);
-	if (ret)
+
+	if ((ret == -ERESTARTSYS) || (ret == -ENXIO))
 		scsi_cd_put(cd);
 
-out:
+	return ret;
+}
+
+static int sr_block_open_finish(struct block_device *bdev, fmode_t mode,
+				int ret)
+{
+	struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
+
+	/* wait for drive to get ready */
+	if ((ret == -ENOMEDIUM) && !(mode & FMODE_NDELAY)) {
+		struct scsi_device *sdev = cd->device;
+		/*
+		 * Cannot use sr_block_ioctl because it locks sr_mutex blocking
+		 * out any processes trying to access the drive
+		 */
+		scsi_autopm_get_device(sdev);
+		cdrom_autoclose(&cd->cdi);
+		ret = __sr_block_open(bdev, mode);
+		scsi_autopm_put_device(sdev);
+	}
+
 	return ret;
 }
 
@@ -640,6 +669,7 @@ static const struct block_device_operations sr_bdops =
 {
 	.owner		= THIS_MODULE,
 	.open		= sr_block_open,
+	.open_finish	= sr_block_open_finish,
 	.release	= sr_block_release,
 	.ioctl		= sr_block_ioctl,
 	.check_events	= sr_block_check_events,
-- 
2.23.0

Re: [PATCH v4 rebase 00/10] Fix cdrom autoclose

[Jens Axboe]

On 11/26/19 12:54 PM, Michal Suchanek wrote:
> Hello,
> 
> there is cdrom autoclose feature that is supposed to close the tray,
> wait for the disc to become ready, and then open the device.
> 
> This used to work in ancient times. Then in old times there was a hack
> in util-linux which worked around the breakage which probably resulted
> from switching to scsi emulation.
> 
> Currently util-linux maintainer refuses to merge another hack on the
> basis that kernel still has the feature so it should be fixed there.
> The code needs not be replicated in every userspace utility like mount
> or dd which has no business knowing which devices are CD-roms and where
> the autoclose setting is in the kernel.
> 
> This is rebase on top of current master.
> 
> Also it seems that most people think that this is fix for WMware because
> there is one patch dealing with WMware.

I think the main complaint with this is that it's kind of a stretch to
add core functionality for a device type that's barely being
manufactured anymore and is mostly used in a virtualized fashion. I
think it you could fix this without 10 patches of churn and without
adding a new ->open() addition to fops, then people would be a lot more
receptive to the idea of improving cdrom auto-close.

-- 
Jens Axboe

Re: [PATCH v4 rebase 00/10] Fix cdrom autoclose

[Michal Suchánek]

On Tue, Nov 26, 2019 at 01:01:42PM -0700, Jens Axboe wrote:
> On 11/26/19 12:54 PM, Michal Suchanek wrote:
> > Hello,
> > 
> > there is cdrom autoclose feature that is supposed to close the tray,
> > wait for the disc to become ready, and then open the device.
> > 
> > This used to work in ancient times. Then in old times there was a hack
> > in util-linux which worked around the breakage which probably resulted
> > from switching to scsi emulation.
> > 
> > Currently util-linux maintainer refuses to merge another hack on the
> > basis that kernel still has the feature so it should be fixed there.
> > The code needs not be replicated in every userspace utility like mount
> > or dd which has no business knowing which devices are CD-roms and where
> > the autoclose setting is in the kernel.
> > 
> > This is rebase on top of current master.
> > 
> > Also it seems that most people think that this is fix for WMware because
> > there is one patch dealing with WMware.
> 
> I think the main complaint with this is that it's kind of a stretch to
> add core functionality for a device type that's barely being
> manufactured anymore and is mostly used in a virtualized fashion. I
> think it you could fix this without 10 patches of churn and without
> adding a new ->open() addition to fops, then people would be a lot more
> receptive to the idea of improving cdrom auto-close.

I see no way to do that cleanly.

There are two open modes for cdrom devices - blocking and non-blocking.

In blocking mode open() should analyze the medium so that it's ready
when it returns. In non-blocking mode it should return immediately so
long as you can talk to the device.

When waiting in open() with locks held the processes trying to open the
device are locked out regradless of the mode they use.

The only way to solve this is to pretend that the device is open and do
the wait afterwards with the device unlocked.

Thanks

Michal

Re: [PATCH v4 rebase 00/10] Fix cdrom autoclose

[Jens Axboe]

On 11/26/19 1:21 PM, Michal Suchánek wrote:
> On Tue, Nov 26, 2019 at 01:01:42PM -0700, Jens Axboe wrote:
>> On 11/26/19 12:54 PM, Michal Suchanek wrote:
>>> Hello,
>>>
>>> there is cdrom autoclose feature that is supposed to close the tray,
>>> wait for the disc to become ready, and then open the device.
>>>
>>> This used to work in ancient times. Then in old times there was a hack
>>> in util-linux which worked around the breakage which probably resulted
>>> from switching to scsi emulation.
>>>
>>> Currently util-linux maintainer refuses to merge another hack on the
>>> basis that kernel still has the feature so it should be fixed there.
>>> The code needs not be replicated in every userspace utility like mount
>>> or dd which has no business knowing which devices are CD-roms and where
>>> the autoclose setting is in the kernel.
>>>
>>> This is rebase on top of current master.
>>>
>>> Also it seems that most people think that this is fix for WMware because
>>> there is one patch dealing with WMware.
>>
>> I think the main complaint with this is that it's kind of a stretch to
>> add core functionality for a device type that's barely being
>> manufactured anymore and is mostly used in a virtualized fashion. I
>> think it you could fix this without 10 patches of churn and without
>> adding a new ->open() addition to fops, then people would be a lot more
>> receptive to the idea of improving cdrom auto-close.
> 
> I see no way to do that cleanly.
> 
> There are two open modes for cdrom devices - blocking and
> non-blocking.
> 
> In blocking mode open() should analyze the medium so that it's ready
> when it returns. In non-blocking mode it should return immediately so
> long as you can talk to the device.
> 
> When waiting in open() with locks held the processes trying to open
> the device are locked out regradless of the mode they use.
> 
> The only way to solve this is to pretend that the device is open and
> do the wait afterwards with the device unlocked.

How is this any different from an open on a file that needs to bring in
meta data on a busy rotating device, which can also take seconds?

-- 
Jens Axboe

Re: [PATCH v4 rebase 00/10] Fix cdrom autoclose

[Michal Suchánek]

On Tue, Nov 26, 2019 at 04:13:32PM -0700, Jens Axboe wrote:
> On 11/26/19 1:21 PM, Michal Suchánek wrote:
> > On Tue, Nov 26, 2019 at 01:01:42PM -0700, Jens Axboe wrote:
> >> On 11/26/19 12:54 PM, Michal Suchanek wrote:
> >>> Hello,
> >>>
> >>> there is cdrom autoclose feature that is supposed to close the tray,
> >>> wait for the disc to become ready, and then open the device.
> >>>
> >>> This used to work in ancient times. Then in old times there was a hack
> >>> in util-linux which worked around the breakage which probably resulted
> >>> from switching to scsi emulation.
> >>>
> >>> Currently util-linux maintainer refuses to merge another hack on the
> >>> basis that kernel still has the feature so it should be fixed there.
> >>> The code needs not be replicated in every userspace utility like mount
> >>> or dd which has no business knowing which devices are CD-roms and where
> >>> the autoclose setting is in the kernel.
> >>>
> >>> This is rebase on top of current master.
> >>>
> >>> Also it seems that most people think that this is fix for WMware because
> >>> there is one patch dealing with WMware.
> >>
> >> I think the main complaint with this is that it's kind of a stretch to
> >> add core functionality for a device type that's barely being
> >> manufactured anymore and is mostly used in a virtualized fashion. I
> >> think it you could fix this without 10 patches of churn and without
> >> adding a new ->open() addition to fops, then people would be a lot more
> >> receptive to the idea of improving cdrom auto-close.
> > 
> > I see no way to do that cleanly.
> > 
> > There are two open modes for cdrom devices - blocking and
> > non-blocking.
> > 
> > In blocking mode open() should analyze the medium so that it's ready
> > when it returns. In non-blocking mode it should return immediately so
> > long as you can talk to the device.
> > 
> > When waiting in open() with locks held the processes trying to open
> > the device are locked out regradless of the mode they use.
> > 
> > The only way to solve this is to pretend that the device is open and
> > do the wait afterwards with the device unlocked.
> 
> How is this any different from an open on a file that needs to bring in
> meta data on a busy rotating device, which can also take seconds?

First, accessing a file will take seconds only when your system is
seriously overloaded or misconfigured. The access time for rotational
storage is tens of milliseconds. With cdrom the access time after
closing the door is measured in tens of seconds on common hardware. It
can be shorter but also possibly longer. I am not aware of any limit
there. It may be reasonable to want to get device status during this
time.

Second, fetching the metadata for the file does not block operations that
don't need the metadata. Here waiting for the drive to get ready blocks
all access. You could get drive status if you did not try to open it
but once you do you can no longer talk to it.

Thanks

Michal

Re: [PATCH v4 rebase 00/10] Fix cdrom autoclose

[Michal Suchánek]

On Wed, Nov 27, 2019 at 09:11:44AM +0100, Michal Suchánek wrote:
> On Tue, Nov 26, 2019 at 04:13:32PM -0700, Jens Axboe wrote:
> > On 11/26/19 1:21 PM, Michal Suchánek wrote:
> > > On Tue, Nov 26, 2019 at 01:01:42PM -0700, Jens Axboe wrote:
> > >> On 11/26/19 12:54 PM, Michal Suchanek wrote:
> > >>> Hello,
> > >>>
> > >>> there is cdrom autoclose feature that is supposed to close the tray,
> > >>> wait for the disc to become ready, and then open the device.
> > >>>
> > >>> This used to work in ancient times. Then in old times there was a hack
> > >>> in util-linux which worked around the breakage which probably resulted
> > >>> from switching to scsi emulation.
> > >>>
> > >>> Currently util-linux maintainer refuses to merge another hack on the
> > >>> basis that kernel still has the feature so it should be fixed there.
> > >>> The code needs not be replicated in every userspace utility like mount
> > >>> or dd which has no business knowing which devices are CD-roms and where
> > >>> the autoclose setting is in the kernel.
> > >>>
> > >>> This is rebase on top of current master.
> > >>>
> > >>> Also it seems that most people think that this is fix for WMware because
> > >>> there is one patch dealing with WMware.
> > >>
> > >> I think the main complaint with this is that it's kind of a stretch to
> > >> add core functionality for a device type that's barely being
> > >> manufactured anymore and is mostly used in a virtualized fashion. I

That optical drives are hardly manufactured is kind of a stretch. I have
no problem obtaining drives from a few manufacturers in any nearby
computer store. While using DVDs may be slowly getting out of fashion
the same applies to all optical drives, including Blueray. Some of these
will stay for forseeable future.

> > >> think it you could fix this without 10 patches of churn and without
> > >> adding a new ->open() addition to fops, then people would be a lot more
> > >> receptive to the idea of improving cdrom auto-close.
> > > 
> > > I see no way to do that cleanly.
> > > 
> > > There are two open modes for cdrom devices - blocking and
> > > non-blocking.
> > > 
> > > In blocking mode open() should analyze the medium so that it's ready
> > > when it returns. In non-blocking mode it should return immediately so
> > > long as you can talk to the device.
> > > 
> > > When waiting in open() with locks held the processes trying to open
> > > the device are locked out regradless of the mode they use.
> > > 
> > > The only way to solve this is to pretend that the device is open and
> > > do the wait afterwards with the device unlocked.
> > 
> > How is this any different from an open on a file that needs to bring in
> > meta data on a busy rotating device, which can also take seconds?
> 
> First, accessing a file will take seconds only when your system is
> seriously overloaded or misconfigured. The access time for rotational
> storage is tens of milliseconds. With cdrom the access time after
> closing the door is measured in tens of seconds on common hardware. It
> can be shorter but also possibly longer. I am not aware of any limit
> there. It may be reasonable to want to get device status during this
> time.
> 
> Second, fetching the metadata for the file does not block operations that
> don't need the metadata. Here waiting for the drive to get ready blocks
> all access. You could get drive status if you did not try to open it
> but once you do you can no longer talk to it.

So let's look at the alternatives. One proposed alternative was to
change the locking calls to the locks that are held while waiting in
open() to interuptible so that impatient users can at least kill
processes waiting on their CD medium to become ready.

What is held are sr_mutex and bd_mutex.

bd_mutex is per_device so any open() or close() on the same CD-ROM
device is blocked. There are a number of other sites where bd_mutex is
locked and it will be needed to figure out which of these can be called
with a cd-rom device and change them to killable so that processes
waiting on the lock to don't get uninterriptibly stuck. This may be more
code churn than this patchset. I think we can exclude loop.c and
zram-dev.c but ioctl.c, xen-blkfront.c, and block_dev.c apply. Don't
know about dasd.

The bd_mutex is held in iterate_bdevs so all bets are off wrt being able
to operate the system.  Once a process is stuck waiting in blkdev_get()
which calls open() on the cdrom you cannot iterate block devices. With
boot times measured in seconds and medium analysis times measured in
tens of seconds users will not be amused.

autoclose defaults to on, and blkid reads deviced in blocking mode
causing all the fun stuff to trigger (patch pending to change that).
Nonetheless any number of utilities still not aware of this nonblock
quirk out there will try to open the device sooner or later blocking all
operations that require iterating the list of block devices.

Adding tens of seconds to block device opening time (which I assume
might need iterating list of block devices) might even overflow some
systemd timeout and fail boot.  There is timeout for each particular job
but there are also cumulative timeouts for something like 'locate device
with this UUID' which are fixed regardles of the number of layers (LVM,
md, ..) involved.

The other approach is to do like harddisks. With a harddisk a medium is
'fixed' - that is assumed to be present always. Any error accessing the
medium is reported on read() or write() and not necessarily on open().
This would require hooking the autoclose to the operations that require
actual medium - probably something like count_tracks(), and eschew
calling these from open(). That would work but breaks the contract
described in the current API documentation - that is if you don't open
with O_NONBLOCK and there is obvious medium error like no medium at all
or no usable track you get the error on open() rather than on whatever
opration that tries to use the track.

Thanks

Michal