Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] drivers/cdrom: improved ioctl for media change detection
To:     Lukas Prediger <lumip@lumip.de>
Cc:     linux-kernel@vger.kernel.org
References: <20210805194417.12439-1-lumip@lumip.de>
 <6d6c533d-465e-33ee-5801-cb7ea84924a8@kernel.dk>
 <f0d33ff3-6b9d-bbe7-1776-a22f9f271155@lumip.de>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <5f3b7d8b-9e97-094b-efd1-cad6cab793b6@kernel.dk>
Date:   Fri, 27 Aug 2021 21:18:16 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <f0d33ff3-6b9d-bbe7-1776-a22f9f271155@lumip.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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