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

[PATCH] 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>
---
This is my first patch sent to the kernel and I followed the recommended
process as closely as I could. If I misstepped somewhere, please let me know.
I also tried to find a mailing list more specific to the problem but was unable,
please inform me if there is one where this should be sent instead.

Finally, I wasn't sure whether/how to add my name to the updated docs
and if I should update the date on them (there have been some recent commits
that did not result in credits given to the commit author or updating
the dates in the docs, but those did not add any new content either), so
info in the most appropriate way would be welcome.

Looking forward to your feedback.
---
 Documentation/cdrom/cdrom-standard.rst      | 14 +++++-
 Documentation/userspace-api/ioctl/cdrom.rst |  6 ++-
 drivers/cdrom/cdrom.c                       | 56 +++++++++++++++++++--
 include/linux/cdrom.h                       |  1 +
 include/uapi/linux/cdrom.h                  | 15 ++++++
 5 files changed, 86 insertions(+), 6 deletions(-)

diff --git a/Documentation/cdrom/cdrom-standard.rst b/Documentation/cdrom/cdrom-standard.rst
index 5845960ca382..8b219ba9b427 100644
--- a/Documentation/cdrom/cdrom-standard.rst
+++ b/Documentation/cdrom/cdrom-standard.rst
@@ -3,9 +3,10 @@ A Linux CD-ROM standard
 =======================
 
 :Author: David van Leeuwen <david@ElseWare.cistron.nl>
-:Date: 12 March 1999
+:Date: 5 August 2021
 :Updated by: Erik Andersen (andersee@debian.org)
 :Updated by: Jens Axboe (axboe@image.dk)
+:Updated by: Lukas Prediger (lumip@lumip.de)
 
 
 Introduction
@@ -907,6 +908,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
+	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..59305d04d034 100644
--- a/Documentation/userspace-api/ioctl/cdrom.rst
+++ b/Documentation/userspace-api/ioctl/cdrom.rst
@@ -3,8 +3,9 @@ Summary of CDROM ioctl calls
 ============================
 
 - Edward A. Falk <efalk@google.com>
+- Lukas Prediger <lumip@lumip.de>
 
-November, 2004
+August, 2021
 
 This document attempts to describe the ioctl(2) calls supported by
 the CDROM layer.  These are by-and-large implemented (as of Linux 2.6)
@@ -54,6 +55,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..af0721c96e06 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_time = ktime_get_mono_fast_ns();
+}
+
 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_time = 0;
 
 	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 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_time);
+	tmp_info.last_media_change = cdi->last_media_change_time;
+
+	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..20ac81e182fc 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;
+	ktime_t last_media_change_time;
 };
 
 struct cdrom_device_ops {
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 6c34f6e2f1f7..9df481845b9e 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
+{
+	__u64   last_media_change;	/* Timestamp of the last detected media
+					 * change. May be set by caller, updated
+					 * upon successful return of ioctl.
+					 */
+	__u8    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] drivers/cdrom: improved ioctl for media change detection

[Lukas Prediger]

On 05.08.21 22:44, 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.
>
> Signed-off-by: Lukas Prediger <lumip@lumip.de>
> ---
> This is my first patch sent to the kernel and I followed the recommended
> process as closely as I could. If I misstepped somewhere, please let me know.
> I also tried to find a mailing list more specific to the problem but was unable,
> please inform me if there is one where this should be sent instead.
>
> Finally, I wasn't sure whether/how to add my name to the updated docs
> and if I should update the date on them (there have been some recent commits
> that did not result in credits given to the commit author or updating
> the dates in the docs, but those did not add any new content either), so
> info in the most appropriate way would be welcome.
>
> Looking forward to your feedback.
> ---
>  Documentation/cdrom/cdrom-standard.rst      | 14 +++++-
>  Documentation/userspace-api/ioctl/cdrom.rst |  6 ++-
>  drivers/cdrom/cdrom.c                       | 56 +++++++++++++++++++--
>  include/linux/cdrom.h                       |  1 +
>  include/uapi/linux/cdrom.h                  | 15 ++++++
>  5 files changed, 86 insertions(+), 6 deletions(-)

As it has been several weeks and I haven't heard anything at all in response to this, 
I'd like to bring this to your attention once more. If something is wrong with it,
please let me know so I can fix it.
- Lukas

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

[Jens Axboe]

On 8/5/21 1:44 PM, 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.
> 
> Signed-off-by: Lukas Prediger <lumip@lumip.de>
> ---
> This is my first patch sent to the kernel and I followed the recommended
> process as closely as I could. If I misstepped somewhere, please let me know.
> I also tried to find a mailing list more specific to the problem but was unable,
> please inform me if there is one where this should be sent instead.
> 
> Finally, I wasn't sure whether/how to add my name to the updated docs
> and if I should update the date on them (there have been some recent commits
> that did not result in credits given to the commit author or updating
> the dates in the docs, but those did not add any new content either), so
> info in the most appropriate way would be welcome.

Thanks for sending in the patch, appologies it's taken so long to get a
response. CDROM isn't really actively maintained much these days,
unfortunately. Should get a new maintainer.

Anyway, for this patch, few comments inline.


> diff --git a/Documentation/cdrom/cdrom-standard.rst b/Documentation/cdrom/cdrom-standard.rst
> index 5845960ca382..8b219ba9b427 100644
> --- a/Documentation/cdrom/cdrom-standard.rst
> +++ b/Documentation/cdrom/cdrom-standard.rst
> @@ -3,9 +3,10 @@ A Linux CD-ROM standard
>  =======================
>  
>  :Author: David van Leeuwen <david@ElseWare.cistron.nl>
> -:Date: 12 March 1999
> +:Date: 5 August 2021
>  :Updated by: Erik Andersen (andersee@debian.org)
>  :Updated by: Jens Axboe (axboe@image.dk)
> +:Updated by: Lukas Prediger (lumip@lumip.de)

Drop these changes, this is from a time from before git and revision
history, no need to update them further.

> diff --git a/Documentation/userspace-api/ioctl/cdrom.rst b/Documentation/userspace-api/ioctl/cdrom.rst
> index 3b4c0506de46..59305d04d034 100644
> --- a/Documentation/userspace-api/ioctl/cdrom.rst
> +++ b/Documentation/userspace-api/ioctl/cdrom.rst
> @@ -3,8 +3,9 @@ Summary of CDROM ioctl calls
>  ============================
>  
>  - Edward A. Falk <efalk@google.com>
> +- Lukas Prediger <lumip@lumip.de>
>  
> -November, 2004
> +August, 2021

Ditto

> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index feb827eefd1a..af0721c96e06 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_time = ktime_get_mono_fast_ns();
> +}

I'd just use jiffies for this, it's not really a case of something that
needs a fine grained clock source. That'll give you 1-10ms resolution,
which should be more than adequate for this. Then use jiffies_to_msecs()
and make the API be in miliseconds.

> @@ -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 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_time);

If you make the jiffies change, you can use time_before() etc on these
which avoids the wrap.

> @@ -295,6 +297,19 @@ struct cdrom_generic_command
>  	};
>  };
>  
> +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> +struct cdrom_timed_media_change_info
> +{
> +	__u64   last_media_change;	/* Timestamp of the last detected media
> +					 * change. May be set by caller, updated
> +					 * upon successful return of ioctl.
> +					 */
> +	__u8    has_changed;		/* Set to 1 by ioctl if last detected media
> +					 * change was more recent than
> +					 * last_media_change set by caller.
> +					 */
> +};

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. Though the latter
probably isn't much of a concern here, but...

-- 
Jens Axboe

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

[Lukas Prediger]

On 27.08.21 01:38, Jens Axboe wrote:

> Thanks for sending in the patch, appologies it's taken so long to get a
> response. CDROM isn't really actively maintained much these days,
> unfortunately. Should get a new maintainer.
>
> Anyway, for this patch, few comments inline.

Thanks for the feedback, I'm currently editing the patch to improve it based on your suggestions.
Some questions before resubmitting below.

> I'd just use jiffies for this, it's not really a case of something that
> needs a fine grained clock source. That'll give you 1-10ms resolution,
> which should be more than adequate for this. Then use jiffies_to_msecs()
> and make the API be in miliseconds.

>> @@ -295,6 +297,19 @@ struct cdrom_generic_command
>>  	};
>>  };
>>  
>> +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
>> +struct cdrom_timed_media_change_info
>> +{
>> +	__u64   last_media_change;	/* Timestamp of the last detected media
>> +					 * change. May be set by caller, updated
>> +					 * upon successful return of ioctl.
>> +					 */
>> +	__u8    has_changed;		/* Set to 1 by ioctl if last detected media
>> +					 * change was more recent than
>> +					 * last_media_change set by caller.
>> +					 */
>> +};
>>
> 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. Though the latter
> probably isn't much of a concern here, but...

1. jiffies_to_msecs returns unsigned int. Should I reflect that in the struct (i.e., make the
last_media_change and has_changed fields also of type unsigned int or should I keep them at
a fixed bit width?

2. As the last_media_change field will be in ms now, is it safe to convert those back to jiffies
for comparison or is there a risk of information loss (due to rounding or whatever) in either conversion?
More technically, can I make the assumption that for any jiffies value x it holds that

time_before(msecs_to_jiffies(jiffies_to_msecs(x)), x) is always false ?

I tried to determine that from the code, but I haven't reached a conclusion that I'm confident in..

Kind regards,
Lukas

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

[Jens Axboe]

On 8/27/21 11:30 AM, Lukas Prediger wrote:
>>> @@ -295,6 +297,19 @@ struct cdrom_generic_command
>>>  	};
>>>  };
>>>  
>>> +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
>>> +struct cdrom_timed_media_change_info
>>> +{
>>> +	__u64   last_media_change;	/* Timestamp of the last detected media
>>> +					 * change. May be set by caller, updated
>>> +					 * upon successful return of ioctl.
>>> +					 */
>>> +	__u8    has_changed;		/* Set to 1 by ioctl if last detected media
>>> +					 * change was more recent than
>>> +					 * last_media_change set by caller.
>>> +					 */
>>> +};
>>>
>> 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. Though the latter
>> probably isn't much of a concern here, but...
> 
> 1. jiffies_to_msecs returns unsigned int. Should I reflect that in the
> struct (i.e., make the last_media_change and has_changed fields also
> of type unsigned int or should I keep them at a fixed bit width?

You can make it an u32. Always use explicitly sized types for user API
structures.

> 2. As the last_media_change field will be in ms now, is it safe to
> convert those back to jiffies for comparison or is there a risk of
> information loss (due to rounding or whatever) in either conversion?
> More technically, can I make the assumption that for any jiffies value
> x it holds that

The granularity of jiffies depends on the HZ setting, generally just
consider it somewhere in between 100 and 1000. That's where my initial
granularity numbers came from.

> time_before(msecs_to_jiffies(jiffies_to_msecs(x)), x) is always false ?

I don't think that matters. Internally, always keep things in jiffies.
That's what you use to compare with, and to check if it's changed since
last time. The only time you convert to ms is to pass it back to
userspace. And that's going to be a delta of jiffies always, just ensure
you assign last_checked = jiffies when it's setup initially.

-- 
Jens Axboe

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

[Lukas Prediger]

Thanks for the reply and sorry for the spam :/. I am not sure what 
happened there.

>> 2. As the last_media_change field will be in ms now, is it safe to
>> convert those back to jiffies for comparison or is there a risk of
>> information loss (due to rounding or whatever) in either conversion?
>> More technically, can I make the assumption that for any jiffies value
>> x it holds that
> The granularity of jiffies depends on the HZ setting, generally just
> consider it somewhere in between 100 and 1000. That's where my initial
> granularity numbers came from.
>
>> time_before(msecs_to_jiffies(jiffies_to_msecs(x)), x) is always false ?
> I don't think that matters. Internally, always keep things in jiffies.
> That's what you use to compare with, and to check if it's changed since
> last time. The only time you convert to ms is to pass it back to
> userspace. And that's going to be a delta of jiffies always, just ensure
> you assign last_checked = jiffies when it's setup initially.
>
The issue I have is that the value I am comparing to is provided by the code
calling the ioctl so that I don't have to maintain state for every potential
calling process in the kernel. Therefore, if we want the API to work with ms,
I have to convert the user provided value back to jiffies for comparison. 

I now ran a brief test that suggests that the above condition does not hold
and therefore the value returned in has_changed may be 1 for subsequent calls
when the disc was not in fact changed.

Workarounds I see would be to either expose the jiffies value through the
API (which is maybe not really clean), or making the comparison on the ms value
(but how to deal with potential wraparounds then?). Of those, I would currently
tend to the first and treat the nature of the returned timestamp as an opaque
value from the user perspective - it is probably not really of any use to them
outside of passing it back into the ioctl for subsequent calls. Do you
see other ways to resolve this I may not have thought of?

Kind regards,
Lukas

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

[Jens Axboe]

On 8/28/21 4:27 AM, Lukas Prediger wrote:
> Thanks for the reply and sorry for the spam :/. I am not sure what 
> happened there.
> 
>>> 2. As the last_media_change field will be in ms now, is it safe to
>>> convert those back to jiffies for comparison or is there a risk of
>>> information loss (due to rounding or whatever) in either conversion?
>>> More technically, can I make the assumption that for any jiffies value
>>> x it holds that
>> The granularity of jiffies depends on the HZ setting, generally just
>> consider it somewhere in between 100 and 1000. That's where my initial
>> granularity numbers came from.
>>
>>> time_before(msecs_to_jiffies(jiffies_to_msecs(x)), x) is always false ?
>> I don't think that matters. Internally, always keep things in jiffies.
>> That's what you use to compare with, and to check if it's changed since
>> last time. The only time you convert to ms is to pass it back to
>> userspace. And that's going to be a delta of jiffies always, just ensure
>> you assign last_checked = jiffies when it's setup initially.
>>
> The issue I have is that the value I am comparing to is provided by
> the code calling the ioctl so that I don't have to maintain state for
> every potential calling process in the kernel. Therefore, if we want
> the API to work with ms, I have to convert the user provided value
> back to jiffies for comparison. 
> 
> I now ran a brief test that suggests that the above condition does not
> hold and therefore the value returned in has_changed may be 1 for
> subsequent calls when the disc was not in fact changed.
> 
> Workarounds I see would be to either expose the jiffies value through
> the API (which is maybe not really clean), or making the comparison on
> the ms value (but how to deal with potential wraparounds then?). Of
> those, I would currently tend to the first and treat the nature of the
> returned timestamp as an opaque value from the user perspective - it
> is probably not really of any use to them outside of passing it back
> into the ioctl for subsequent calls. Do you see other ways to resolve
> this I may not have thought of?

Maybe it's better to just use ms internally too, and avoid the whole
conversion side of things. Hence just use ktime_get() and ktime_to_ms().

-- 
Jens Axboe

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

[Lukas Prediger]

Hey Jens,

not sure if it has escaped your notice since I didn't put
it as a reply to this message, but I have submitted an
updated version of the patch a few days ago:
https://lore.kernel.org/lkml/20210829143735.512146-1-lumip@lumip.de/

Best regards,
Lukas