Re: [PATCH v2] cdrom, scsi: sr: propagate read-only status to block layer via set_disk_ro()

[Daan De Meyer]

Hi Phil,

Thanks for the review! You can adjust the submission. Yeah I should
probably fix my smtp stuff to use my corporate address. Feel free to
adjust the submission as you suggested.

Thanks!

Daan

On Sun, 26 Apr 2026 at 23:03, Phillip Potter <phil@xxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Apr 22, 2026 at 11:32:06AM +0000, Daan De Meyer wrote:
> >
> >  drivers/cdrom/cdrom.c | 73 ++++++++++++++++++++++++++++---------------
> >  drivers/scsi/sr.c     | 11 ++-----
> >  drivers/scsi/sr.h     |  1 -
> >  include/linux/cdrom.h |  1 +
> >  4 files changed, 51 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> > index fc049612d6dc..62934cf4b10d 100644
> > --- a/drivers/cdrom/cdrom.c
> > +++ b/drivers/cdrom/cdrom.c
> > @@ -631,6 +631,16 @@ int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
> >
> >       WARN_ON(!cdo->generic_packet);
> >
> > +     /*
> > +      * Propagate the drive's write support to the block layer so BLKROGET
> > +      * reflects actual write capability. Drivers that use GET CONFIGURATION
> > +      * features (CDC_MRW_W, CDC_RAM) must have called
> > +      * cdrom_probe_write_features() before register_cdrom() so the mask is
> > +      * complete here.
> > +      */
> > +     set_disk_ro(disk, !CDROM_CAN(CDC_DVD_RAM | CDC_MRW_W | CDC_RAM |
> > +                                  CDC_CD_RW));
> > +
> >       cd_dbg(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
> >       mutex_lock(&cdrom_mutex);
> >       list_add(&cdi->list, &cdrom_list);
> > @@ -742,6 +752,44 @@ static int cdrom_is_random_writable(struct cdrom_device_info *cdi, int *write)
> >       return 0;
> >  }
> >
> > +/*
> > + * Probe write-related MMC features via GET CONFIGURATION and update
> > + * cdi->mask accordingly. Drivers that populate cdi->mask from the MODE SENSE
> > + * capabilities page (e.g. sr) should call this after those MODE SENSE bits
> > + * have been set but before register_cdrom(), so that the full set of
> > + * write-capability bits is known by the time register_cdrom() decides on the
> > + * initial read-only state of the disk.
> > + */
> > +void cdrom_probe_write_features(struct cdrom_device_info *cdi)
> > +{
> > +     int mrw, mrw_write, ram_write;
> > +
> > +     mrw = 0;
> > +     if (!cdrom_is_mrw(cdi, &mrw_write))
> > +             mrw = 1;
> > +
> > +     if (CDROM_CAN(CDC_MO_DRIVE))
> > +             ram_write = 1;
> > +     else
> > +             (void) cdrom_is_random_writable(cdi, &ram_write);
> > +
> > +     if (mrw)
> > +             cdi->mask &= ~CDC_MRW;
> > +     else
> > +             cdi->mask |= CDC_MRW;
> > +
> > +     if (mrw_write)
> > +             cdi->mask &= ~CDC_MRW_W;
> > +     else
> > +             cdi->mask |= CDC_MRW_W;
> > +
> > +     if (ram_write)
> > +             cdi->mask &= ~CDC_RAM;
> > +     else
> > +             cdi->mask |= CDC_RAM;
> > +}
> > +EXPORT_SYMBOL(cdrom_probe_write_features);
> > +
> >  static int cdrom_media_erasable(struct cdrom_device_info *cdi)
> >  {
> >       disc_information di;
> > @@ -894,33 +942,8 @@ static int cdrom_is_dvd_rw(struct cdrom_device_info *cdi)
> >   */
> >  static int cdrom_open_write(struct cdrom_device_info *cdi)
> >  {
> > -     int mrw, mrw_write, ram_write;
> >       int ret = 1;
> >
> > -     mrw = 0;
> > -     if (!cdrom_is_mrw(cdi, &mrw_write))
> > -             mrw = 1;
> > -
> > -     if (CDROM_CAN(CDC_MO_DRIVE))
> > -             ram_write = 1;
> > -     else
> > -             (void) cdrom_is_random_writable(cdi, &ram_write);
> > -
> > -     if (mrw)
> > -             cdi->mask &= ~CDC_MRW;
> > -     else
> > -             cdi->mask |= CDC_MRW;
> > -
> > -     if (mrw_write)
> > -             cdi->mask &= ~CDC_MRW_W;
> > -     else
> > -             cdi->mask |= CDC_MRW_W;
> > -
> > -     if (ram_write)
> > -             cdi->mask &= ~CDC_RAM;
> > -     else
> > -             cdi->mask |= CDC_RAM;
> > -
> >       if (CDROM_CAN(CDC_MRW_W))
> >               ret = cdrom_mrw_open_write(cdi);
> >       else if (CDROM_CAN(CDC_DVD_RAM))
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 7adb2573f50d..c36c54ecd354 100644
> > --- a/drivers/scsi/sr.c
> > +++ b/drivers/scsi/sr.c
> > @@ -395,7 +395,7 @@ static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt)
> >
> >       switch (req_op(rq)) {
> >       case REQ_OP_WRITE:
> > -             if (!cd->writeable)
> > +             if (get_disk_ro(cd->disk))
> >                       goto out;
> >               SCpnt->cmnd[0] = WRITE_10;
> >               cd->cdi.media_written = 1;
> > @@ -681,6 +681,7 @@ static int sr_probe(struct scsi_device *sdev)
> >       error = -ENOMEM;
> >       if (get_capabilities(cd))
> >               goto fail_minor;
> > +     cdrom_probe_write_features(&cd->cdi);
> >       sr_vendor_init(cd);
> >
> >       set_capacity(disk, cd->capacity);
> > @@ -899,14 +900,6 @@ static int get_capabilities(struct scsi_cd *cd)
> >       /*else    I don't think it can close its tray
> >               cd->cdi.mask |= CDC_CLOSE_TRAY; */
> >
> > -     /*
> > -      * if DVD-RAM, MRW-W or CD-RW, we are randomly writable
> > -      */
> > -     if ((cd->cdi.mask & (CDC_DVD_RAM | CDC_MRW_W | CDC_RAM | CDC_CD_RW)) !=
> > -                     (CDC_DVD_RAM | CDC_MRW_W | CDC_RAM | CDC_CD_RW)) {
> > -             cd->writeable = 1;
> > -     }
> > -
> >       kfree(buffer);
> >       return 0;
> >  }
> > diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
> > index dc899277b3a4..2d92f9cb6fec 100644
> > --- a/drivers/scsi/sr.h
> > +++ b/drivers/scsi/sr.h
> > @@ -35,7 +35,6 @@ typedef struct scsi_cd {
> >       struct scsi_device *device;
> >       unsigned int vendor;    /* vendor code, see sr_vendor.c         */
> >       unsigned long ms_offset;        /* for reading multisession-CD's        */
> > -     unsigned writeable : 1;
> >       unsigned use:1;         /* is this device still supportable     */
> >       unsigned xa_flag:1;     /* CD has XA sectors ? */
> >       unsigned readcd_known:1;        /* drive supports READ_CD (0xbe) */
> > diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
> > index b907e6c2307d..260d7968cf72 100644
> > --- a/include/linux/cdrom.h
> > +++ b/include/linux/cdrom.h
> > @@ -108,6 +108,7 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
> >  extern unsigned int cdrom_check_events(struct cdrom_device_info *cdi,
> >                                      unsigned int clearing);
> >
> > +extern void cdrom_probe_write_features(struct cdrom_device_info *cdi);
> >  extern int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi);
> >  extern void unregister_cdrom(struct cdrom_device_info *cdi);
> >
> > --
> > 2.53.0
> >
>
> Hi Daan,
>
> I've looked through the patch and it looks good to me. Looks like a
> decent change. I think in this case too, it's unlikely this historically
> broken behaviour is being relied upon (i.e. it seems unlikely to me that
> fixing it would break anything).
>
> In addition, I've build tested and booted/run some read/write tests with
> your patch which worked fine for me too.
>
> Reviewed-by: Phillip Potter <phil@xxxxxxxxxxxxxxxx>
>
> One final question from me though, and a purely procedural one:
> The patch was submitted from your gmail address, but signed off by your
> corporate address. Are you happy for me to adjust the submission so that
> the author appears as your corporate address when sending on for
> inclusion? Let me know, thanks.
>
> Regards,
> Phil