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;
Date:   Mon, 6 Sep 2021 23:57:24 +0100
From:   Phillip Potter <phil@philpotter.co.uk>
To:     Lukas Prediger <lumip@lumip.de>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
        linux-block@vger.kernel.org
Subject: Re: [PATCH v2] drivers/cdrom: improved ioctl for media change
 detection
Message-ID: <YTac1M6+pdwZ532J@equinox>
References: <20210829143735.512146-1-lumip@lumip.de>
 <65bf6d1a-f65d-910c-60c7-0a4911a52e9a@kernel.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <65bf6d1a-f65d-910c-60c7-0a4911a52e9a@kernel.dk>
Precedence: bulk

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