Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 15936 bytes --]

CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210829143735.512146-1-lumip@lumip.de>
References: <20210829143735.512146-1-lumip@lumip.de>
TO: Lukas Prediger <lumip@lumip.de>
TO: axboe(a)kernel.dk
CC: linux-kernel(a)vger.kernel.org
CC: Lukas Prediger <lumip@lumip.de>

Hi Lukas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.14 next-20210831]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lukas-Prediger/drivers-cdrom-improved-ioctl-for-media-change-detection/20210829-224230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git c54b245d011855ea91c5beff07f1db74143ce614
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: arm-randconfig-c002-20210830 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 4b1fde8a2b681dad2ce0c082a5d6422caa06b0bc)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/7137a773f62ec45b6e321b0bf555d3459ac9510d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Lukas-Prediger/drivers-cdrom-improved-ioctl-for-media-change-detection/20210829-224230
        git checkout 7137a773f62ec45b6e321b0bf555d3459ac9510d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm clang-analyzer 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)
   include/linux/compiler_types.h:308:3: note: expanded from macro '__compiletime_assert'
                   if (!(condition))                                       \
                   ^
   drivers/vhost/scsi.c:2056:31: note: Loop condition is false.  Exiting loop
           struct vhost_scsi_tpg *tpg = container_of(se_tpg,
                                        ^
   include/linux/kernel.h:704:2: note: expanded from macro 'container_of'
           BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
           ^
   include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
   #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                       ^
   include/linux/compiler_types.h:328:2: note: expanded from macro 'compiletime_assert'
           _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
           ^
   include/linux/compiler_types.h:316:2: note: expanded from macro '_compiletime_assert'
           __compiletime_assert(condition, msg, prefix, suffix)
           ^
   include/linux/compiler_types.h:306:2: note: expanded from macro '__compiletime_assert'
           do {                                                            \
           ^
   drivers/vhost/scsi.c:2056:2: note: 'tpg' initialized to a null pointer value
           struct vhost_scsi_tpg *tpg = container_of(se_tpg,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/vhost/scsi.c:2059:12: note: Calling 'kstrtoul'
           int ret = kstrtoul(page, 0, &val);
                     ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:219:58: note: Left side of '&&' is false
           if (sizeof(unsigned long) == sizeof(unsigned long long) &&
                                                                   ^
   include/linux/kernel.h:223:3: note: Returning value, which participates in a condition later
                   return _kstrtoul(s, base, res);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/vhost/scsi.c:2059:12: note: Returning from 'kstrtoul'
           int ret = kstrtoul(page, 0, &val);
                     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/vhost/scsi.c:2061:6: note: Assuming 'ret' is 0
           if (ret) {
               ^~~
   drivers/vhost/scsi.c:2061:2: note: Taking false branch
           if (ret) {
           ^
   drivers/vhost/scsi.c:2065:6: note: Assuming 'val' is equal to 0
           if (val != 0 && val != 1 && val != 3) {
               ^~~~~~~~
   drivers/vhost/scsi.c:2065:15: note: Left side of '&&' is false
           if (val != 0 && val != 1 && val != 3) {
                        ^
   drivers/vhost/scsi.c:2069:27: note: Access to field 'tv_fabric_prot_type' results in a dereference of a null pointer (loaded from variable 'tpg')
           tpg->tv_fabric_prot_type = val;
           ~~~                      ^
   Suppressed 3 warnings (3 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   3 warnings generated.
   Suppressed 3 warnings (3 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   drivers/block/paride/fit3.c:86:13: warning: Value stored to 'b' is never read [clang-analyzer-deadcode.DeadStores]
                   a = r4(); b = r4(); 
                             ^
   drivers/block/paride/fit3.c:86:13: note: Value stored to 'b' is never read
   Suppressed 3 warnings (3 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   3 warnings generated.
   Suppressed 3 warnings (3 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   3 warnings generated.
   Suppressed 3 warnings (3 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   3 warnings generated.
   Suppressed 3 warnings (3 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   drivers/block/paride/pd.c:348:9: warning: Although the value stored to 'block' is used in the enclosing expression, the value is never actually read from 'block' [clang-analyzer-deadcode.DeadStores]
                   h = ((block >>= 8) & 15) + 0x40;
                         ^         ~
   drivers/block/paride/pd.c:348:9: note: Although the value stored to 'block' is used in the enclosing expression, the value is never actually read from 'block'
                   h = ((block >>= 8) & 15) + 0x40;
                         ^         ~
   drivers/block/paride/pd.c:353:9: warning: Although the value stored to 'block' is used in the enclosing expression, the value is never actually read from 'block' [clang-analyzer-deadcode.DeadStores]
                   c1 = (block >>= 8);
                         ^         ~
   drivers/block/paride/pd.c:353:9: note: Although the value stored to 'block' is used in the enclosing expression, the value is never actually read from 'block'
                   c1 = (block >>= 8);
                         ^         ~
   drivers/block/paride/pd.c:686:3: warning: Value stored to 'r' is never read [clang-analyzer-deadcode.DeadStores]
                   r = pd_wait_for(disk, STAT_READY, DBMSG("RDY after VRFY"));
                   ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/block/paride/pd.c:686:3: note: Value stored to 'r' is never read
                   r = pd_wait_for(disk, STAT_READY, DBMSG("RDY after VRFY"));
                   ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/block/paride/pd.c:886:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
           strcpy(p->disk_name, disk->name);
           ^~~~~~
   drivers/block/paride/pd.c:886:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
           strcpy(p->disk_name, disk->name);
           ^~~~~~
   Suppressed 3 warnings (3 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   9 warnings generated.
>> drivers/block/paride/pcd.c:193:8: warning: Excessive padding in 'struct pcd_unit' (8 padding bytes, where 0 is optimal). 
   Optimal fields order: 
   info, 
   pi, 
   drive, 
   last_sense, 
   changed, 
   present, 
   name, 
   disk, 
   rq_list, 
   pia, 
   tag_set, 
   consider reordering the fields or adding explicit padding members [clang-analyzer-optin.performance.Padding]
   struct pcd_unit {
   ~~~~~~~^~~~~~~~~~
   drivers/block/paride/pcd.c:193:8: note: Excessive padding in 'struct pcd_unit' (8 padding bytes, where 0 is optimal). Optimal fields order: info, pi, drive, last_sense, changed, present, name, disk, rq_list, pia, tag_set, consider reordering the fields or adding explicit padding members
   struct pcd_unit {
   ~~~~~~~^~~~~~~~~~
   drivers/block/paride/pcd.c:346:3: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
                   strcpy(disk->disk_name, cd->name);      /* umm... */
                   ^~~~~~
   drivers/block/paride/pcd.c:346:3: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
                   strcpy(disk->disk_name, cd->name);      /* umm... */
                   ^~~~~~
   drivers/block/paride/pcd.c:503:33: warning: The left operand of '&' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
                                  cd->name, fun, buf[2] & 0xf, buf[12], buf[13]);
                                                        ^
   drivers/block/paride/pcd.c:1010:6: note: Assuming 'disable' is 0
           if (disable)
               ^~~~~~~
   drivers/block/paride/pcd.c:1010:2: note: Taking false branch
           if (disable)
           ^
   drivers/block/paride/pcd.c:1015:2: note: Taking false branch
           if (pcd_detect())
           ^
   drivers/block/paride/pcd.c:1019:2: note: Calling 'pcd_probe_capabilities'
           pcd_probe_capabilities();
           ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/block/paride/pcd.c:689:2: note: Loop condition is true.  Entering loop body
           for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++) {
           ^
   drivers/block/paride/pcd.c:690:12: note: Field 'present' is 1
                   if (!cd->present)
                            ^
   drivers/block/paride/pcd.c:690:3: note: Taking false branch
                   if (!cd->present)
                   ^
   drivers/block/paride/pcd.c:692:7: note: Calling 'pcd_atapi'
                   r = pcd_atapi(cd, cmd, 18, buffer, "mode sense capabilities");
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/block/paride/pcd.c:516:6: note: Calling 'pcd_command'
           r = pcd_command(cd, cmd, dlen, fun);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/block/paride/pcd.c:410:6: note: Assuming the condition is true
           if (pcd_wait(cd, IDE_BUSY | IDE_DRQ, 0, fun, "before command")) {
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/block/paride/pcd.c:410:2: note: Taking true branch
           if (pcd_wait(cd, IDE_BUSY | IDE_DRQ, 0, fun, "before command")) {
           ^
   drivers/block/paride/pcd.c:412:3: note: Returning the value -1, which participates in a condition later
                   return -1;
                   ^~~~~~~~~
   drivers/block/paride/pcd.c:516:6: note: Returning from 'pcd_command'
           r = pcd_command(cd, cmd, dlen, fun);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/block/paride/pcd.c:517:2: note: Left side of '&&' is true
           mdelay(1);
           ^
   include/linux/delay.h:44:3: note: expanded from macro 'mdelay'
           (__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
            ^
   drivers/block/paride/pcd.c:517:2: note: 1 is <= 2
           mdelay(1);
           ^
   include/linux/delay.h:44:30: note: expanded from macro 'mdelay'
           (__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
                                       ^~~
   drivers/block/paride/pcd.c:517:2: note: '?' condition is true
           mdelay(1);
           ^
   include/linux/delay.h:44:2: note: expanded from macro 'mdelay'
           (__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
           ^
   drivers/block/paride/pcd.c:517:2: note: '?' condition is true
           mdelay(1);
           ^
   include/linux/delay.h:44:52: note: expanded from macro 'mdelay'
           (__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
                                                             ^
   arch/arm/include/asm/delay.h:83:3: note: expanded from macro 'udelay'
           (__builtin_constant_p(n) ?                                      \
            ^
   drivers/block/paride/pcd.c:517:2: note: '?' condition is false
           mdelay(1);
           ^
   include/linux/delay.h:44:52: note: expanded from macro 'mdelay'
           (__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
                                                             ^
   arch/arm/include/asm/delay.h:84:5: note: expanded from macro 'udelay'

vim +193 drivers/block/paride/pcd.c

^1da177e4c3f41 Linus Torvalds 2005-04-16  192  
^1da177e4c3f41 Linus Torvalds 2005-04-16 @193  struct pcd_unit {
^1da177e4c3f41 Linus Torvalds 2005-04-16  194  	struct pi_adapter pia;	/* interface to paride layer */
^1da177e4c3f41 Linus Torvalds 2005-04-16  195  	struct pi_adapter *pi;
^1da177e4c3f41 Linus Torvalds 2005-04-16  196  	int drive;		/* master/slave */
^1da177e4c3f41 Linus Torvalds 2005-04-16  197  	int last_sense;		/* result of last request sense */
^1da177e4c3f41 Linus Torvalds 2005-04-16  198  	int changed;		/* media change seen */
^1da177e4c3f41 Linus Torvalds 2005-04-16  199  	int present;		/* does this unit exist ? */
^1da177e4c3f41 Linus Torvalds 2005-04-16  200  	char *name;		/* pcd0, pcd1, etc */
^1da177e4c3f41 Linus Torvalds 2005-04-16  201  	struct cdrom_device_info info;	/* uniform cdrom interface */
^1da177e4c3f41 Linus Torvalds 2005-04-16  202  	struct gendisk *disk;
89c6b165096933 Jens Axboe     2018-10-15  203  	struct blk_mq_tag_set tag_set;
89c6b165096933 Jens Axboe     2018-10-15  204  	struct list_head rq_list;
^1da177e4c3f41 Linus Torvalds 2005-04-16  205  };
^1da177e4c3f41 Linus Torvalds 2005-04-16  206  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32584 bytes --]

[PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Lukas Prediger]

The current implementation of the CDROM_MEDIA_CHANGED ioctl relies on
global state, meaning that only one process can detect a disc change
while the ioctl call will return 0 for other calling processes afterwards
(see bug 213267 ).

This introduces a new cdrom ioctl, CDROM_TIMED_MEDIA_CHANGE, that
works by maintaining a timestamp of the last detected disc change instead
of a boolean flag: Processes calling this ioctl command can provide
a timestamp of the last disc change known to them and receive
an indication whether the disc was changed since then and the updated
timestamp.

I considered fixing the buggy behavior in the original
CDROM_MEDIA_CHANGED ioctl but that would require maintaining state
for each calling process in the kernel, which seems like a worse
solution than introducing this new ioctl.

Signed-off-by: Lukas Prediger <lumip@lumip.de>
---
Second version based on your feedback for the first
attempt. Please let me know if further changes are required.
---
 Documentation/cdrom/cdrom-standard.rst      | 11 ++++
 Documentation/userspace-api/ioctl/cdrom.rst |  3 ++
 drivers/cdrom/cdrom.c                       | 56 +++++++++++++++++++--
 include/linux/cdrom.h                       |  1 +
 include/uapi/linux/cdrom.h                  | 15 ++++++
 5 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/Documentation/cdrom/cdrom-standard.rst b/Documentation/cdrom/cdrom-standard.rst
index 5845960ca382..52ea7b6b2fe8 100644
--- a/Documentation/cdrom/cdrom-standard.rst
+++ b/Documentation/cdrom/cdrom-standard.rst
@@ -907,6 +907,17 @@ commands can be identified by the underscores in their names.
 	specifies the slot for which the information is given. The special
 	value *CDSL_CURRENT* requests that information about the currently
 	selected slot be returned.
+`CDROM_TIMED_MEDIA_CHANGE`
+	Checks whether the disc has been changed since a user supplied time
+	and returns the time of the last disc change.
+
+	*arg* is a pointer to a *cdrom_timed_media_change_info* struct.
+	*arg->last_media_change* may be set by calling code to signal
+	the timestamp of the last known media change (by the caller).
+	Upon successful return, this ioctl call will set
+	*arg->last_media_change* to the latest media change timestamp (in ms)
+	known by the kernel/driver and set *arg->has_changed* to 1 if
+	that timestamp is more recent than the timestamp set by the caller.
 `CDROM_DRIVE_STATUS`
 	Returns the status of the drive by a call to
 	*drive_status()*. Return values are defined in cdrom_drive_status_.
diff --git a/Documentation/userspace-api/ioctl/cdrom.rst b/Documentation/userspace-api/ioctl/cdrom.rst
index 3b4c0506de46..bac5bbf93ca0 100644
--- a/Documentation/userspace-api/ioctl/cdrom.rst
+++ b/Documentation/userspace-api/ioctl/cdrom.rst
@@ -54,6 +54,9 @@ are as follows:
 	CDROM_SELECT_SPEED	Set the CD-ROM speed
 	CDROM_SELECT_DISC	Select disc (for juke-boxes)
 	CDROM_MEDIA_CHANGED	Check is media changed
+	CDROM_TIMED_MEDIA_CHANGE	Check if media changed
+					since given time
+					(struct cdrom_timed_media_change_info)
 	CDROM_DRIVE_STATUS	Get tray position, etc.
 	CDROM_DISC_STATUS	Get disc type, etc.
 	CDROM_CHANGER_NSLOTS	Get number of slots
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index feb827eefd1a..a040a867f6a2 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -344,6 +344,12 @@ static void cdrom_sysctl_register(void);
 
 static LIST_HEAD(cdrom_list);
 
+static void signal_media_change(struct cdrom_device_info *cdi)
+{
+	cdi->mc_flags = 0x3; /* set media changed bits, on both queues */
+	cdi->last_media_change_ms = ktime_to_ms(ktime_get());
+}
+
 int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
 			       struct packet_command *cgc)
 {
@@ -616,6 +622,7 @@ int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
 	ENSURE(cdo, generic_packet, CDC_GENERIC_PACKET);
 	cdi->mc_flags = 0;
 	cdi->options = CDO_USE_FFLAGS;
+	cdi->last_media_change_ms = ktime_to_ms(ktime_get());
 
 	if (autoclose == 1 && CDROM_CAN(CDC_CLOSE_TRAY))
 		cdi->options |= (int) CDO_AUTO_CLOSE;
@@ -1421,8 +1428,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
 		cdi->ops->check_events(cdi, 0, slot);
 
 	if (slot == CDSL_NONE) {
-		/* set media changed bits, on both queues */
-		cdi->mc_flags = 0x3;
+		signal_media_change(cdi);
 		return cdrom_load_unload(cdi, -1);
 	}
 
@@ -1455,7 +1461,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
 		slot = curslot;
 
 	/* set media changed bits on both queues */
-	cdi->mc_flags = 0x3;
+	signal_media_change(cdi);
 	if ((ret = cdrom_load_unload(cdi, slot)))
 		return ret;
 
@@ -1521,7 +1527,7 @@ int media_changed(struct cdrom_device_info *cdi, int queue)
 	cdi->ioctl_events = 0;
 
 	if (changed) {
-		cdi->mc_flags = 0x3;    /* set bit on both queues */
+		signal_media_change(cdi);
 		ret |= 1;
 		cdi->media_written = 0;
 	}
@@ -2391,6 +2397,46 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
 	return ret;
 }
 
+/*
+ * Media change detection with timing information.
+ *
+ * arg is a pointer to a cdrom_timed_media_change_info struct.
+ * arg->last_media_change may be set by calling code to signal
+ * the timestamp (in ms) of the last known media change (by the caller).
+ * Upon successful return, ioctl call will set arg->last_media_change
+ * to the latest media change timestamp known by the kernel/driver
+ * and set arg->has_changed to 1 if that timestamp is more recent
+ * than the timestamp set by the caller.
+ */
+static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi,
+		unsigned long arg)
+{
+	int ret;
+	struct cdrom_timed_media_change_info __user *info;
+	struct cdrom_timed_media_change_info tmp_info;
+
+	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
+		return -ENOSYS;
+
+	info = (struct cdrom_timed_media_change_info __user *)arg;
+	cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n");
+
+	ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT);
+	if (ret < 0)
+		return ret;
+
+	if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0)
+		return -EFAULT;
+
+	tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0);
+	tmp_info.last_media_change = cdi->last_media_change_ms;
+
+	if (copy_to_user(info, &tmp_info, sizeof(*info)) != 0)
+		return -EFAULT;
+
+	return 0;
+}
+
 static int cdrom_ioctl_set_options(struct cdrom_device_info *cdi,
 		unsigned long arg)
 {
@@ -3375,6 +3421,8 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
 		return cdrom_ioctl_eject_sw(cdi, arg);
 	case CDROM_MEDIA_CHANGED:
 		return cdrom_ioctl_media_changed(cdi, arg);
+	case CDROM_TIMED_MEDIA_CHANGE:
+		return cdrom_ioctl_timed_media_change(cdi, arg);
 	case CDROM_SET_OPTIONS:
 		return cdrom_ioctl_set_options(cdi, arg);
 	case CDROM_CLEAR_OPTIONS:
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index f48d0a31deae..7c10b564db29 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -64,6 +64,7 @@ struct cdrom_device_info {
 	int for_data;
 	int (*exit)(struct cdrom_device_info *);
 	int mrw_mode_page;
+	__s64 last_media_change_ms;
 };
 
 struct cdrom_device_ops {
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 6c34f6e2f1f7..9b17868667ed 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -147,6 +147,8 @@
 #define CDROM_NEXT_WRITABLE	0x5394	/* get next writable block */
 #define CDROM_LAST_WRITTEN	0x5395	/* get last block written on disc */
 
+#define CDROM_TIMED_MEDIA_CHANGE   0x5396  /* get the timestamp of the last media change */
+
 /*******************************************************
  * CDROM IOCTL structures
  *******************************************************/
@@ -295,6 +297,19 @@ struct cdrom_generic_command
 	};
 };
 
+/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
+struct cdrom_timed_media_change_info
+{
+	__s64	last_media_change;	/* Timestamp of the last detected media
+					 * change in ms. May be set by caller, updated
+					 * upon successful return of ioctl.
+					 */
+	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
+					 * change was more recent than
+					 * last_media_change set by caller.
+					 */
+};
+
 /*
  * A CD-ROM physical sector size is 2048, 2052, 2056, 2324, 2332, 2336, 
  * 2340, or 2352 bytes long.  
-- 
2.25.1

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Jens Axboe]

On 8/29/21 8:37 AM, Lukas Prediger wrote:
> The current implementation of the CDROM_MEDIA_CHANGED ioctl relies on
> global state, meaning that only one process can detect a disc change
> while the ioctl call will return 0 for other calling processes afterwards
> (see bug 213267 ).
> 
> This introduces a new cdrom ioctl, CDROM_TIMED_MEDIA_CHANGE, that
> works by maintaining a timestamp of the last detected disc change instead
> of a boolean flag: Processes calling this ioctl command can provide
> a timestamp of the last disc change known to them and receive
> an indication whether the disc was changed since then and the updated
> timestamp.
> 
> I considered fixing the buggy behavior in the original
> CDROM_MEDIA_CHANGED ioctl but that would require maintaining state
> for each calling process in the kernel, which seems like a worse
> solution than introducing this new ioctl.

This looks pretty good to me now. Adding Phillip to the CC, he's the new
CDROM maintainer. Leaving the rest of the message below intact because
of that.


> Signed-off-by: Lukas Prediger <lumip@lumip.de>
> ---
> Second version based on your feedback for the first
> attempt. Please let me know if further changes are required.
> ---
>  Documentation/cdrom/cdrom-standard.rst      | 11 ++++
>  Documentation/userspace-api/ioctl/cdrom.rst |  3 ++
>  drivers/cdrom/cdrom.c                       | 56 +++++++++++++++++++--
>  include/linux/cdrom.h                       |  1 +
>  include/uapi/linux/cdrom.h                  | 15 ++++++
>  5 files changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/cdrom/cdrom-standard.rst b/Documentation/cdrom/cdrom-standard.rst
> index 5845960ca382..52ea7b6b2fe8 100644
> --- a/Documentation/cdrom/cdrom-standard.rst
> +++ b/Documentation/cdrom/cdrom-standard.rst
> @@ -907,6 +907,17 @@ commands can be identified by the underscores in their names.
>  	specifies the slot for which the information is given. The special
>  	value *CDSL_CURRENT* requests that information about the currently
>  	selected slot be returned.
> +`CDROM_TIMED_MEDIA_CHANGE`
> +	Checks whether the disc has been changed since a user supplied time
> +	and returns the time of the last disc change.
> +
> +	*arg* is a pointer to a *cdrom_timed_media_change_info* struct.
> +	*arg->last_media_change* may be set by calling code to signal
> +	the timestamp of the last known media change (by the caller).
> +	Upon successful return, this ioctl call will set
> +	*arg->last_media_change* to the latest media change timestamp (in ms)
> +	known by the kernel/driver and set *arg->has_changed* to 1 if
> +	that timestamp is more recent than the timestamp set by the caller.
>  `CDROM_DRIVE_STATUS`
>  	Returns the status of the drive by a call to
>  	*drive_status()*. Return values are defined in cdrom_drive_status_.
> diff --git a/Documentation/userspace-api/ioctl/cdrom.rst b/Documentation/userspace-api/ioctl/cdrom.rst
> index 3b4c0506de46..bac5bbf93ca0 100644
> --- a/Documentation/userspace-api/ioctl/cdrom.rst
> +++ b/Documentation/userspace-api/ioctl/cdrom.rst
> @@ -54,6 +54,9 @@ are as follows:
>  	CDROM_SELECT_SPEED	Set the CD-ROM speed
>  	CDROM_SELECT_DISC	Select disc (for juke-boxes)
>  	CDROM_MEDIA_CHANGED	Check is media changed
> +	CDROM_TIMED_MEDIA_CHANGE	Check if media changed
> +					since given time
> +					(struct cdrom_timed_media_change_info)
>  	CDROM_DRIVE_STATUS	Get tray position, etc.
>  	CDROM_DISC_STATUS	Get disc type, etc.
>  	CDROM_CHANGER_NSLOTS	Get number of slots
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index feb827eefd1a..a040a867f6a2 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -344,6 +344,12 @@ static void cdrom_sysctl_register(void);
>  
>  static LIST_HEAD(cdrom_list);
>  
> +static void signal_media_change(struct cdrom_device_info *cdi)
> +{
> +	cdi->mc_flags = 0x3; /* set media changed bits, on both queues */
> +	cdi->last_media_change_ms = ktime_to_ms(ktime_get());
> +}
> +
>  int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
>  			       struct packet_command *cgc)
>  {
> @@ -616,6 +622,7 @@ int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
>  	ENSURE(cdo, generic_packet, CDC_GENERIC_PACKET);
>  	cdi->mc_flags = 0;
>  	cdi->options = CDO_USE_FFLAGS;
> +	cdi->last_media_change_ms = ktime_to_ms(ktime_get());
>  
>  	if (autoclose == 1 && CDROM_CAN(CDC_CLOSE_TRAY))
>  		cdi->options |= (int) CDO_AUTO_CLOSE;
> @@ -1421,8 +1428,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
>  		cdi->ops->check_events(cdi, 0, slot);
>  
>  	if (slot == CDSL_NONE) {
> -		/* set media changed bits, on both queues */
> -		cdi->mc_flags = 0x3;
> +		signal_media_change(cdi);
>  		return cdrom_load_unload(cdi, -1);
>  	}
>  
> @@ -1455,7 +1461,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
>  		slot = curslot;
>  
>  	/* set media changed bits on both queues */
> -	cdi->mc_flags = 0x3;
> +	signal_media_change(cdi);
>  	if ((ret = cdrom_load_unload(cdi, slot)))
>  		return ret;
>  
> @@ -1521,7 +1527,7 @@ int media_changed(struct cdrom_device_info *cdi, int queue)
>  	cdi->ioctl_events = 0;
>  
>  	if (changed) {
> -		cdi->mc_flags = 0x3;    /* set bit on both queues */
> +		signal_media_change(cdi);
>  		ret |= 1;
>  		cdi->media_written = 0;
>  	}
> @@ -2391,6 +2397,46 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
>  	return ret;
>  }
>  
> +/*
> + * Media change detection with timing information.
> + *
> + * arg is a pointer to a cdrom_timed_media_change_info struct.
> + * arg->last_media_change may be set by calling code to signal
> + * the timestamp (in ms) of the last known media change (by the caller).
> + * Upon successful return, ioctl call will set arg->last_media_change
> + * to the latest media change timestamp known by the kernel/driver
> + * and set arg->has_changed to 1 if that timestamp is more recent
> + * than the timestamp set by the caller.
> + */
> +static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi,
> +		unsigned long arg)
> +{
> +	int ret;
> +	struct cdrom_timed_media_change_info __user *info;
> +	struct cdrom_timed_media_change_info tmp_info;
> +
> +	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
> +		return -ENOSYS;
> +
> +	info = (struct cdrom_timed_media_change_info __user *)arg;
> +	cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n");
> +
> +	ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0)
> +		return -EFAULT;
> +
> +	tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0);
> +	tmp_info.last_media_change = cdi->last_media_change_ms;
> +
> +	if (copy_to_user(info, &tmp_info, sizeof(*info)) != 0)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
>  static int cdrom_ioctl_set_options(struct cdrom_device_info *cdi,
>  		unsigned long arg)
>  {
> @@ -3375,6 +3421,8 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
>  		return cdrom_ioctl_eject_sw(cdi, arg);
>  	case CDROM_MEDIA_CHANGED:
>  		return cdrom_ioctl_media_changed(cdi, arg);
> +	case CDROM_TIMED_MEDIA_CHANGE:
> +		return cdrom_ioctl_timed_media_change(cdi, arg);
>  	case CDROM_SET_OPTIONS:
>  		return cdrom_ioctl_set_options(cdi, arg);
>  	case CDROM_CLEAR_OPTIONS:
> diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
> index f48d0a31deae..7c10b564db29 100644
> --- a/include/linux/cdrom.h
> +++ b/include/linux/cdrom.h
> @@ -64,6 +64,7 @@ struct cdrom_device_info {
>  	int for_data;
>  	int (*exit)(struct cdrom_device_info *);
>  	int mrw_mode_page;
> +	__s64 last_media_change_ms;
>  };
>  
>  struct cdrom_device_ops {
> diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
> index 6c34f6e2f1f7..9b17868667ed 100644
> --- a/include/uapi/linux/cdrom.h
> +++ b/include/uapi/linux/cdrom.h
> @@ -147,6 +147,8 @@
>  #define CDROM_NEXT_WRITABLE	0x5394	/* get next writable block */
>  #define CDROM_LAST_WRITTEN	0x5395	/* get last block written on disc */
>  
> +#define CDROM_TIMED_MEDIA_CHANGE   0x5396  /* get the timestamp of the last media change */
> +
>  /*******************************************************
>   * CDROM IOCTL structures
>   *******************************************************/
> @@ -295,6 +297,19 @@ struct cdrom_generic_command
>  	};
>  };
>  
> +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> +struct cdrom_timed_media_change_info
> +{
> +	__s64	last_media_change;	/* Timestamp of the last detected media
> +					 * change in ms. May be set by caller, updated
> +					 * upon successful return of ioctl.
> +					 */
> +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
> +					 * change was more recent than
> +					 * last_media_change set by caller.
> +					 */
> +};
> +
>  /*
>   * A CD-ROM physical sector size is 2048, 2052, 2056, 2324, 2332, 2336, 
>   * 2340, or 2352 bytes long.  
> 


-- 
Jens Axboe

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Phillip Potter]

On Mon, Sep 06, 2021 at 02:11:31PM -0600, Jens Axboe wrote:
> On 8/29/21 8:37 AM, Lukas Prediger wrote:
> > The current implementation of the CDROM_MEDIA_CHANGED ioctl relies on
> > global state, meaning that only one process can detect a disc change
> > while the ioctl call will return 0 for other calling processes afterwards
> > (see bug 213267 ).
> > 
> > This introduces a new cdrom ioctl, CDROM_TIMED_MEDIA_CHANGE, that
> > works by maintaining a timestamp of the last detected disc change instead
> > of a boolean flag: Processes calling this ioctl command can provide
> > a timestamp of the last disc change known to them and receive
> > an indication whether the disc was changed since then and the updated
> > timestamp.
> > 
> > I considered fixing the buggy behavior in the original
> > CDROM_MEDIA_CHANGED ioctl but that would require maintaining state
> > for each calling process in the kernel, which seems like a worse
> > solution than introducing this new ioctl.
> 
> This looks pretty good to me now. Adding Phillip to the CC, he's the new
> CDROM maintainer. Leaving the rest of the message below intact because
> of that.
> 
> >
...
> >

Dear Lukas,

Thank you for the patch, much appreciated and looks great. One very
minor thing though has jumped out at me after running checkpatch though:

> > --- a/include/uapi/linux/cdrom.h
> > +++ b/include/uapi/linux/cdrom.h
> > @@ -147,6 +147,8 @@
> >  #define CDROM_NEXT_WRITABLE  0x5394  /* get next writable block */
> >  #define CDROM_LAST_WRITTEN   0x5395  /* get last block written on disc */
> >
> > +#define CDROM_TIMED_MEDIA_CHANGE   0x5396  /* get the timestamp of the last media change */
> > +
> >  /*******************************************************
> >   * CDROM IOCTL structures
> >   *******************************************************/
> > @@ -295,6 +297,19 @@ struct cdrom_generic_command
> >       };
> >  };
> >
> > +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> > +struct cdrom_timed_media_change_info
> > +{

This opening brace should be on the same line as the struct definition
as per current style rules.

I also got a checkpatch warning about ENOSYS being used as an error
value, but I can see this usage is standard in the driver and not a
problem so no issue with that.

I will review and test properly after work tomorrow (being new to the
role I'd like to make sure I'm taking the proper time), but I have no
doubt it will work fine. Assuming it does I will be happy to accept the
patch with the above brace change. Thanks again.

> > +	__s64	last_media_change;	/* Timestamp of the last detected media
> > +					 * change in ms. May be set by caller, updated
> > +					 * upon successful return of ioctl.
> > +					 */
> > +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
> > +					 * change was more recent than
> > +					 * last_media_change set by caller.
> > +					 */
> > +};
> > +
> >  /*
> >   * A CD-ROM physical sector size is 2048, 2052, 2056, 2324, 2332, 2336, 
> >   * 2340, or 2352 bytes long.  
> > 

Regards,
Phil

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Phillip Potter]

On Mon, Sep 06, 2021 at 02:11:31PM -0600, Jens Axboe wrote:
> On 8/29/21 8:37 AM, Lukas Prediger wrote:
> > The current implementation of the CDROM_MEDIA_CHANGED ioctl relies on
> > global state, meaning that only one process can detect a disc change
> > while the ioctl call will return 0 for other calling processes afterwards
> > (see bug 213267 ).
> > 
> > This introduces a new cdrom ioctl, CDROM_TIMED_MEDIA_CHANGE, that
> > works by maintaining a timestamp of the last detected disc change instead
> > of a boolean flag: Processes calling this ioctl command can provide
> > a timestamp of the last disc change known to them and receive
> > an indication whether the disc was changed since then and the updated
> > timestamp.
> > 
> > I considered fixing the buggy behavior in the original
> > CDROM_MEDIA_CHANGED ioctl but that would require maintaining state
> > for each calling process in the kernel, which seems like a worse
> > solution than introducing this new ioctl.
> 
> This looks pretty good to me now. Adding Phillip to the CC, he's the new
> CDROM maintainer. Leaving the rest of the message below intact because
> of that.
> 
> 
> > Signed-off-by: Lukas Prediger <lumip@lumip.de>
> > ---
> > Second version based on your feedback for the first
> > attempt. Please let me know if further changes are required.
> > ---
> >  Documentation/cdrom/cdrom-standard.rst      | 11 ++++
> >  Documentation/userspace-api/ioctl/cdrom.rst |  3 ++
> >  drivers/cdrom/cdrom.c                       | 56 +++++++++++++++++++--
> >  include/linux/cdrom.h                       |  1 +
> >  include/uapi/linux/cdrom.h                  | 15 ++++++
> >  5 files changed, 82 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/cdrom/cdrom-standard.rst b/Documentation/cdrom/cdrom-standard.rst
> > index 5845960ca382..52ea7b6b2fe8 100644
> > --- a/Documentation/cdrom/cdrom-standard.rst
> > +++ b/Documentation/cdrom/cdrom-standard.rst
> > @@ -907,6 +907,17 @@ commands can be identified by the underscores in their names.
> >  	specifies the slot for which the information is given. The special
> >  	value *CDSL_CURRENT* requests that information about the currently
> >  	selected slot be returned.
> > +`CDROM_TIMED_MEDIA_CHANGE`
> > +	Checks whether the disc has been changed since a user supplied time
> > +	and returns the time of the last disc change.
> > +
> > +	*arg* is a pointer to a *cdrom_timed_media_change_info* struct.
> > +	*arg->last_media_change* may be set by calling code to signal
> > +	the timestamp of the last known media change (by the caller).
> > +	Upon successful return, this ioctl call will set
> > +	*arg->last_media_change* to the latest media change timestamp (in ms)
> > +	known by the kernel/driver and set *arg->has_changed* to 1 if
> > +	that timestamp is more recent than the timestamp set by the caller.
> >  `CDROM_DRIVE_STATUS`
> >  	Returns the status of the drive by a call to
> >  	*drive_status()*. Return values are defined in cdrom_drive_status_.
> > diff --git a/Documentation/userspace-api/ioctl/cdrom.rst b/Documentation/userspace-api/ioctl/cdrom.rst
> > index 3b4c0506de46..bac5bbf93ca0 100644
> > --- a/Documentation/userspace-api/ioctl/cdrom.rst
> > +++ b/Documentation/userspace-api/ioctl/cdrom.rst
> > @@ -54,6 +54,9 @@ are as follows:
> >  	CDROM_SELECT_SPEED	Set the CD-ROM speed
> >  	CDROM_SELECT_DISC	Select disc (for juke-boxes)
> >  	CDROM_MEDIA_CHANGED	Check is media changed
> > +	CDROM_TIMED_MEDIA_CHANGE	Check if media changed
> > +					since given time
> > +					(struct cdrom_timed_media_change_info)
> >  	CDROM_DRIVE_STATUS	Get tray position, etc.
> >  	CDROM_DISC_STATUS	Get disc type, etc.
> >  	CDROM_CHANGER_NSLOTS	Get number of slots
> > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> > index feb827eefd1a..a040a867f6a2 100644
> > --- a/drivers/cdrom/cdrom.c
> > +++ b/drivers/cdrom/cdrom.c
> > @@ -344,6 +344,12 @@ static void cdrom_sysctl_register(void);
> >  
> >  static LIST_HEAD(cdrom_list);
> >  
> > +static void signal_media_change(struct cdrom_device_info *cdi)
> > +{
> > +	cdi->mc_flags = 0x3; /* set media changed bits, on both queues */
> > +	cdi->last_media_change_ms = ktime_to_ms(ktime_get());
> > +}
> > +
> >  int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
> >  			       struct packet_command *cgc)
> >  {
> > @@ -616,6 +622,7 @@ int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
> >  	ENSURE(cdo, generic_packet, CDC_GENERIC_PACKET);
> >  	cdi->mc_flags = 0;
> >  	cdi->options = CDO_USE_FFLAGS;
> > +	cdi->last_media_change_ms = ktime_to_ms(ktime_get());
> >  
> >  	if (autoclose == 1 && CDROM_CAN(CDC_CLOSE_TRAY))
> >  		cdi->options |= (int) CDO_AUTO_CLOSE;
> > @@ -1421,8 +1428,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
> >  		cdi->ops->check_events(cdi, 0, slot);
> >  
> >  	if (slot == CDSL_NONE) {
> > -		/* set media changed bits, on both queues */
> > -		cdi->mc_flags = 0x3;
> > +		signal_media_change(cdi);
> >  		return cdrom_load_unload(cdi, -1);
> >  	}
> >  
> > @@ -1455,7 +1461,7 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
> >  		slot = curslot;
> >  
> >  	/* set media changed bits on both queues */
> > -	cdi->mc_flags = 0x3;
> > +	signal_media_change(cdi);
> >  	if ((ret = cdrom_load_unload(cdi, slot)))
> >  		return ret;
> >  
> > @@ -1521,7 +1527,7 @@ int media_changed(struct cdrom_device_info *cdi, int queue)
> >  	cdi->ioctl_events = 0;
> >  
> >  	if (changed) {
> > -		cdi->mc_flags = 0x3;    /* set bit on both queues */
> > +		signal_media_change(cdi);
> >  		ret |= 1;
> >  		cdi->media_written = 0;
> >  	}
> > @@ -2391,6 +2397,46 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
> >  	return ret;
> >  }
> >  
> > +/*
> > + * Media change detection with timing information.
> > + *
> > + * arg is a pointer to a cdrom_timed_media_change_info struct.
> > + * arg->last_media_change may be set by calling code to signal
> > + * the timestamp (in ms) of the last known media change (by the caller).
> > + * Upon successful return, ioctl call will set arg->last_media_change
> > + * to the latest media change timestamp known by the kernel/driver
> > + * and set arg->has_changed to 1 if that timestamp is more recent
> > + * than the timestamp set by the caller.
> > + */
> > +static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi,
> > +		unsigned long arg)
> > +{
> > +	int ret;
> > +	struct cdrom_timed_media_change_info __user *info;
> > +	struct cdrom_timed_media_change_info tmp_info;
> > +
> > +	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
> > +		return -ENOSYS;
> > +
> > +	info = (struct cdrom_timed_media_change_info __user *)arg;
> > +	cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n");
> > +
> > +	ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0)
> > +		return -EFAULT;
> > +
> > +	tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0);
> > +	tmp_info.last_media_change = cdi->last_media_change_ms;
> > +
> > +	if (copy_to_user(info, &tmp_info, sizeof(*info)) != 0)
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> >  static int cdrom_ioctl_set_options(struct cdrom_device_info *cdi,
> >  		unsigned long arg)
> >  {
> > @@ -3375,6 +3421,8 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
> >  		return cdrom_ioctl_eject_sw(cdi, arg);
> >  	case CDROM_MEDIA_CHANGED:
> >  		return cdrom_ioctl_media_changed(cdi, arg);
> > +	case CDROM_TIMED_MEDIA_CHANGE:
> > +		return cdrom_ioctl_timed_media_change(cdi, arg);
> >  	case CDROM_SET_OPTIONS:
> >  		return cdrom_ioctl_set_options(cdi, arg);
> >  	case CDROM_CLEAR_OPTIONS:
> > diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
> > index f48d0a31deae..7c10b564db29 100644
> > --- a/include/linux/cdrom.h
> > +++ b/include/linux/cdrom.h
> > @@ -64,6 +64,7 @@ struct cdrom_device_info {
> >  	int for_data;
> >  	int (*exit)(struct cdrom_device_info *);
> >  	int mrw_mode_page;
> > +	__s64 last_media_change_ms;
> >  };
> >  
> >  struct cdrom_device_ops {
> > diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
> > index 6c34f6e2f1f7..9b17868667ed 100644
> > --- a/include/uapi/linux/cdrom.h
> > +++ b/include/uapi/linux/cdrom.h
> > @@ -147,6 +147,8 @@
> >  #define CDROM_NEXT_WRITABLE	0x5394	/* get next writable block */
> >  #define CDROM_LAST_WRITTEN	0x5395	/* get last block written on disc */
> >  
> > +#define CDROM_TIMED_MEDIA_CHANGE   0x5396  /* get the timestamp of the last media change */
> > +
> >  /*******************************************************
> >   * CDROM IOCTL structures
> >   *******************************************************/
> > @@ -295,6 +297,19 @@ struct cdrom_generic_command
> >  	};
> >  };
> >  
> > +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> > +struct cdrom_timed_media_change_info
> > +{
> > +	__s64	last_media_change;	/* Timestamp of the last detected media
> > +					 * change in ms. May be set by caller, updated
> > +					 * upon successful return of ioctl.
> > +					 */
> > +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
> > +					 * change was more recent than
> > +					 * last_media_change set by caller.
> > +					 */
> > +};
> > +
> >  /*
> >   * A CD-ROM physical sector size is 2048, 2052, 2056, 2324, 2332, 2336, 
> >   * 2340, or 2352 bytes long.  
> > 
> 
> 
> -- 
> Jens Axboe
> 

Dear Lukas,

I've now tested your patch, and it works perfectly for me. Also, I have
reviewed the code in more detail and it seems very good. For that
reason, I'm happy to send on to Jens for merging, provided the
previously mentioned struct bracket issue is sorted - as it is so
minor, I can do this for you when I send it on if you're happy with
that, to save you resubmitting? Let me know.

Many thanks for your work on this, much appreciated.

Regards,
Phil

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Lukas Prediger]

Hey Phil,

thanks for taking the time to look at this and give feedback.

> Dear Lukas,
>
> Thank you for the patch, much appreciated and looks great. One very
> minor thing though has jumped out at me after running checkpatch though:
>
> > --- a/include/uapi/linux/cdrom.h
> > +++ b/include/uapi/linux/cdrom.h
> > @@ -147,6 +147,8 @@
> >  #define CDROM_NEXT_WRITABLE  0x5394  /* get next writable block */
> >  #define CDROM_LAST_WRITTEN   0x5395  /* get last block written on disc */
> >
> > +#define CDROM_TIMED_MEDIA_CHANGE   0x5396  /* get the timestamp of the last media change */
> > +
> >  /*******************************************************
> >   * CDROM IOCTL structures
> >   *******************************************************/
> > @@ -295,6 +297,19 @@ struct cdrom_generic_command
> >       };
> >  };
> >
> > +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> > +struct cdrom_timed_media_change_info
> > +{
>
> This opening brace should be on the same line as the struct definition
> as per current style rules.

I also noted that checkpatch reported this and that it is technically a style breach,
however I kept the brace in the newline to be consistent with all the other
cdrom ioctl structs defined above this in the same file for consistency.
I have no strong feelings about this, though, so we could change this.

> I also got a checkpatch warning about ENOSYS being used as an error
> value, but I can see this usage is standard in the driver and not a
> problem so no issue with that.
> 
> I will review and test properly after work tomorrow (being new to the
> role I'd like to make sure I'm taking the proper time), but I have no
> doubt it will work fine. Assuming it does I will be happy to accept the
> patch with the above brace change. Thanks again.
> Regards,
> Phil

Best regards,
Lukas

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Phillip Potter]

On Thu, Sep 09, 2021 at 09:04:28PM +0300, Lukas Prediger wrote:
> Hey Phil,
> 
> thanks for taking the time to look at this and give feedback.
> 

No problem, and great work :-)

> > Dear Lukas,
> >
> > Thank you for the patch, much appreciated and looks great. One very
> > minor thing though has jumped out at me after running checkpatch though:
> >
> > > --- a/include/uapi/linux/cdrom.h
> > > +++ b/include/uapi/linux/cdrom.h
> > > @@ -147,6 +147,8 @@
> > >  #define CDROM_NEXT_WRITABLE  0x5394  /* get next writable block */
> > >  #define CDROM_LAST_WRITTEN   0x5395  /* get last block written on disc */
> > >
> > > +#define CDROM_TIMED_MEDIA_CHANGE   0x5396  /* get the timestamp of the last media change */
> > > +
> > >  /*******************************************************
> > >   * CDROM IOCTL structures
> > >   *******************************************************/
> > > @@ -295,6 +297,19 @@ struct cdrom_generic_command
> > >       };
> > >  };
> > >
> > > +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> > > +struct cdrom_timed_media_change_info
> > > +{
> >
> > This opening brace should be on the same line as the struct definition
> > as per current style rules.
> 
> I also noted that checkpatch reported this and that it is technically a style breach,
> however I kept the brace in the newline to be consistent with all the other
> cdrom ioctl structs defined above this in the same file for consistency.
> I have no strong feelings about this, though, so we could change this.
> 

I take your point, but my personal preference would be to not introduce
code that has new checkpatch errors or warnings, where possible.

Indeed, it may be worth correcting other code in the driver that would
trigger checkpatch violations, and this is something I will look at, provided
it makes sense and isn't too much churn for what are non-semantic changes to
a very stable driver. Many thanks.

Regards,
Phil

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Christoph Hellwig]

> +static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi,
> +		unsigned long arg)
> +{
> +	int ret;
> +	struct cdrom_timed_media_change_info __user *info;
> +	struct cdrom_timed_media_change_info tmp_info;
> +
> +	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
> +		return -ENOSYS;
> +
> +	info = (struct cdrom_timed_media_change_info __user *)arg;
> +	cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n");
> +
> +	ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0)
> +		return -EFAULT;
> +
> +	tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0);

Overly long line here, but more importantly this is much cleaner with
a good old if:


	if (tmp_info.last_media_change - cdi->last_media_change_ms) < 0)
		tmp_info.has_changed = 1;

> +{
> +	__s64	last_media_change;	/* Timestamp of the last detected media
> +					 * change in ms. May be set by caller, updated
> +					 * upon successful return of ioctl.
> +					 */
> +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media

More overly long lines.  Also why is has_changed a u64 if it is used as
a boolean flag?

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Phillip Potter]

> > +static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi,
> > +		unsigned long arg)
> > +{
> > +	int ret;
> > +	struct cdrom_timed_media_change_info __user *info;
> > +	struct cdrom_timed_media_change_info tmp_info;
> > +
> > +	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
> > +		return -ENOSYS;
> > +
> > +	info = (struct cdrom_timed_media_change_info __user *)arg;
> > +	cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n");
> > +
> > +	ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0)
> > +		return -EFAULT;
> > +
> > +	tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0);
>
Dear Christoph,

Sorry didn't see this e-mail before I replied to Lukas, so I hope you
don't think I was ignoring you :-)

> Overly long line here, but more importantly this is much cleaner with
> a good old if:
>
>
>	if (tmp_info.last_media_change - cdi->last_media_change_ms) < 0)
>		tmp_info.has_changed = 1;
>

Whilst I don't disagree this is technically cleaner, the existing style
certainly read well to me. In terms of line length, checkpatch doesn't
complain about it, so I guess you mean purely from a visual perspective?

> > +{
> > +	__s64	last_media_change;	/* Timestamp of the last detected media
> > +					 * change in ms. May be set by caller, updated
> > +					 * upon successful return of ioctl.
> > +					 */
> > +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
>
> More overly long lines.  Also why is has_changed a u64 if it is used as
> a boolean flag?

As this is not a packed struct, would not a smaller value still take up
the same space?

As I'm keen to grow as a maintainer and take feedback, I can certainly
wait for Lukas to reply to this for sure.

Regards,
Phil

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Randy Dunlap]

On 9/8/21 5:17 PM, Phillip Potter wrote:
>>> +static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi,
>>> +		unsigned long arg)
>>> +{
>>> +	int ret;
>>> +	struct cdrom_timed_media_change_info __user *info;
>>> +	struct cdrom_timed_media_change_info tmp_info;
>>> +
>>> +	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
>>> +		return -ENOSYS;
>>> +
>>> +	info = (struct cdrom_timed_media_change_info __user *)arg;
>>> +	cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n");
>>> +
>>> +	ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0)
>>> +		return -EFAULT;
>>> +
>>> +	tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0);
>>
> Dear Christoph,
> 
> Sorry didn't see this e-mail before I replied to Lukas, so I hope you
> don't think I was ignoring you :-)
> 
>> Overly long line here, but more importantly this is much cleaner with
>> a good old if:
>>
>>
>> 	if (tmp_info.last_media_change - cdi->last_media_change_ms) < 0)
>> 		tmp_info.has_changed = 1;
>>
> 
> Whilst I don't disagree this is technically cleaner, the existing style
> certainly read well to me. In terms of line length, checkpatch doesn't
> complain about it, so I guess you mean purely from a visual perspective?

Documentation/process/coding-style.rst says:

   The preferred limit on the length of a single line is 80 columns.

checkpatch only checks lines > 100 columns since that is OK in a few
cases, like a long quoted string.

So try to limit line lengths to 80 columns unless there is some
other reason not to do that.

>>> +{
>>> +	__s64	last_media_change;	/* Timestamp of the last detected media
>>> +					 * change in ms. May be set by caller, updated
>>> +					 * upon successful return of ioctl.
>>> +					 */
>>> +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
>>
>> More overly long lines.  Also why is has_changed a u64 if it is used as
>> a boolean flag?

Might as well be explicit about it and also make it obvious that there
is some space available for other fields.

thanks.
-- 
~Randy

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Lukas Prediger]

Dear Christoph, Phillip and Randy,

thanks to you all for your comments!

>>>> +static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi,
>>>> +		unsigned long arg)
>>>> +{
>>>> +	int ret;
>>>> +	struct cdrom_timed_media_change_info __user *info;
>>>> +	struct cdrom_timed_media_change_info tmp_info;
>>>> +
>>>> +	if (!CDROM_CAN(CDC_MEDIA_CHANGED))
>>>> +		return -ENOSYS;
>>>> +
>>>> +	info = (struct cdrom_timed_media_change_info __user *)arg;
>>>> +	cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n");
>>>> +
>>>> +	ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0)
>>>> +		return -EFAULT;
>>>> +
>>>> +	tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0);
>>>
>>> Overly long line here, but more importantly this is much cleaner with
>>> a good old if:
>>>
>>>
>>> 	if (tmp_info.last_media_change - cdi->last_media_change_ms) < 0)
>>> 		tmp_info.has_changed = 1;
>>>
>> 
>> Whilst I don't disagree this is technically cleaner, the existing style
>> certainly read well to me. 

The if would additionally require to explicitly initialise .has_changed to
zero for the else case, so I favored the single assignment that covers
all cases. I don't have a strong opinion on this, though, so if the if variant
is generally favored, I can change this. (And I will definitely fix the overlength).

>> In terms of line length, checkpatch doesn't
>> complain about it, so I guess you mean purely from a visual perspective?
>
> Documentation/process/coding-style.rst says:
>
>    The preferred limit on the length of a single line is 80 columns.
>
> checkpatch only checks lines > 100 columns since that is OK in a few
> cases, like a long quoted string.
>
> So try to limit line lengths to 80 columns unless there is some
> other reason not to do that.

I wasn't aware that checkpatch.pl does not complain if I exceed the 80 cols,
have fixed those now for an upcoming resubmission.

>>> +{
>>> +	__s64	last_media_change;	/* Timestamp of the last detected media
>>> +					 * change in ms. May be set by caller, updated
>>> +					 * upon successful return of ioctl.
>>> +					 */
>>> +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
>>>
>>> More overly long lines.  Also why is has_changed a u64 if it is used as
>>> a boolean flag?
>>
>> As this is not a packed struct, would not a smaller value still take up
>> the same space?
>
> Might as well be explicit about it and also make it obvious that there
> is some space available for other fields.

I had this as a __u8 in the first submission but Jens asked me to change it.
From his feedback on this:

"The struct layout should be modified such that there are no holes or
padding in it. Probably just make the has_changed a flags thing, and
make it u64 as well. Then you can define bit 0 to be HAS_CHANGED, and
that leaves you room to add more flags in the future."
https://lore.kernel.org/lkml/6d6c533d-465e-33ee-5801-cb7ea84924a8@kernel.dk/

I changed it to __u64 to address this. We could think about turning it
back to a __u8 (or bool) and add some explicit padding members
(a __u8 reserved[3]?), but honestly I don't see much real benefit in that
compared to how it is now.

Best regards,
Lukas

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Phillip Potter]

On Thu, Sep 09, 2021 at 09:05:54PM +0300, Lukas Prediger wrote:
> Dear Christoph, Phillip and Randy,
> 
> thanks to you all for your comments!
> 

Dear Lukas,

You're welcome, thank you for the code.

> >>> Overly long line here, but more importantly this is much cleaner with
> >>> a good old if:
> >>>
> >>>
> >>> 	if (tmp_info.last_media_change - cdi->last_media_change_ms) < 0)
> >>> 		tmp_info.has_changed = 1;
> >>>
> >> 
> >> Whilst I don't disagree this is technically cleaner, the existing style
> >> certainly read well to me. 
> 
> The if would additionally require to explicitly initialise .has_changed to
> zero for the else case, so I favored the single assignment that covers
> all cases. I don't have a strong opinion on this, though, so if the if variant
> is generally favored, I can change this. (And I will definitely fix the overlength).
>

Yes true, but I guess your existing style is harder to split across
lines in a clean way. As mentioned, I didn't mind the original code, but
the line length is a fair point. Your call on this one - those with far
more experience than me would probably argue the if/else form though.

> >> In terms of line length, checkpatch doesn't
> >> complain about it, so I guess you mean purely from a visual perspective?
> >
> > Documentation/process/coding-style.rst says:
> >
> >    The preferred limit on the length of a single line is 80 columns.
> >
> > checkpatch only checks lines > 100 columns since that is OK in a few
> > cases, like a long quoted string.
> >
> > So try to limit line lengths to 80 columns unless there is some
> > other reason not to do that.
> 
> I wasn't aware that checkpatch.pl does not complain if I exceed the 80 cols,
> have fixed those now for an upcoming resubmission.
> 

Same, guilty as charged on this one - live and learn I guess :-)

> >>> +{
> >>> +	__s64	last_media_change;	/* Timestamp of the last detected media
> >>> +					 * change in ms. May be set by caller, updated
> >>> +					 * upon successful return of ioctl.
> >>> +					 */
> >>> +	__u64	has_changed;		/* Set to 1 by ioctl if last detected media
> >>>
> >>> More overly long lines.  Also why is has_changed a u64 if it is used as
> >>> a boolean flag?
> >>
> >> As this is not a packed struct, would not a smaller value still take up
> >> the same space?
> >
> > Might as well be explicit about it and also make it obvious that there
> > is some space available for other fields.
> 
> I had this as a __u8 in the first submission but Jens asked me to change it.
> From his feedback on this:
> 
> "The struct layout should be modified such that there are no holes or
> padding in it. Probably just make the has_changed a flags thing, and
> make it u64 as well. Then you can define bit 0 to be HAS_CHANGED, and
> that leaves you room to add more flags in the future."
> https://lore.kernel.org/lkml/6d6c533d-465e-33ee-5801-cb7ea84924a8@kernel.dk/
> 

Yeah, maybe just a bit more in the comment to emphasize the room for
extra bits in has_changed? I agree it looks fine like this to me though
given the lack of struck packing anyway.

> I changed it to __u64 to address this. We could think about turning it
> back to a __u8 (or bool) and add some explicit padding members
> (a __u8 reserved[3]?), but honestly I don't see much real benefit in that
> compared to how it is now.
> 

I agree with you on this personally, I think it's fine.

Regards,
Phil

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Randy Dunlap]

On 9/9/21 11:05 AM, Lukas Prediger wrote:
>>> As this is not a packed struct, would not a smaller value still take up
>>> the same space?
>> Might as well be explicit about it and also make it obvious that there
>> is some space available for other fields.
> I had this as a __u8 in the first submission but Jens asked me to change it.
>  From his feedback on this:
> 
> "The struct layout should be modified such that there are no holes or
> padding in it. Probably just make the has_changed a flags thing, and
> make it u64 as well. Then you can define bit 0 to be HAS_CHANGED, and
> that leaves you room to add more flags in the future."
> https://lore.kernel.org/lkml/6d6c533d-465e-33ee-5801-cb7ea84924a8@kernel.dk/
> 
> I changed it to __u64 to address this. We could think about turning it
> back to a __u8 (or bool) and add some explicit padding members
> (a __u8 reserved[3]?), but honestly I don't see much real benefit in that
> compared to how it is now.

I have no problem with Jens's suggestion.  It would look more like this:

+{
+	__s64	last_media_change;	/* Timestamp of the last detected media
+					 * change in ms. May be set by caller, updated
+					 * upon successful return of ioctl.
+					 */
+	__u64	media_flags;		/* various <struct> flags */

#define MEDIA_CHANGED			0x1 /* Set to 1 by ioctl if last detected media */
/* other bits of media_flags available for future use */


and not having __u64 has_changed;
which is overkill for a flag.

-- 
~Randy

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Phillip Potter]

On Thu, Sep 09, 2021 at 06:25:35PM -0700, Randy Dunlap wrote:
> I have no problem with Jens's suggestion.  It would look more like this:
> 
> +{
> +	__s64	last_media_change;	/* Timestamp of the last detected media
> +					 * change in ms. May be set by caller, updated
> +					 * upon successful return of ioctl.
> +					 */
> +	__u64	media_flags;		/* various <struct> flags */
> 
> #define MEDIA_CHANGED			0x1 /* Set to 1 by ioctl if last detected media */
> /* other bits of media_flags available for future use */
> 
> 
> and not having __u64 has_changed;
> which is overkill for a flag.
> 
> -- 
> ~Randy
> 

Yeah I like this, good idea. Thanks.

Regards,
Phil

Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

[Phillip Potter]

On Wed, Sep 08, 2021 at 05:42:30PM -0700, Randy Dunlap wrote:
> Documentation/process/coding-style.rst says:
> 
>   The preferred limit on the length of a single line is 80 columns.
> 
> checkpatch only checks lines > 100 columns since that is OK in a few
> cases, like a long quoted string.
> 
> So try to limit line lengths to 80 columns unless there is some
> other reason not to do that.
> 

Dear Randy,

Thank you for clarifying this, appreciate it. Will try and bear it in
mind in future :-)

Regards,
Phil