cdrom: gdrom: fix block I/O and capacity setting

GDROM_DATA_REG is a memory-mapped data register, but the driver uses outsw() and insw() only for this register. Replace this with local helpers using MMIO accessors ioread16_rep() / iowrite16_rep().

Before, it oopsed accessing the data register, as the io_port_base P2SEG gets added to the argument in outsw() / insw(), which leads to an unusable drive:

        BUG: unable to handle kernel paging request at 405f7080
        PC: [<8c28d5b4>] gdrom_spicommand+0x6c/0xb0

Signed-off-by Florian Fuchs <[email protected]>

---
The original Oops can be reproduced just by mounting a disc, like:
mount -t iso9660 -o ro /dev/gdrom /mnt

---
 drivers/cdrom/gdrom.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

drivers/cdrom/gdrom.c +10 -0

index 4ba4dd06cbf4..dccf41fa5d0a 100644
@@ -171,6 +171,16 @@ static void gdrom_identifydevice(void *buf)
                data[c] = __raw_readw(GDROM_DATA_REG);
 }
 
+static void gdrom_fifo_readw(void *buf, unsigned int words)
+{
+       ioread16_rep((void __iomem *)GDROM_DATA_REG, buf, words);
+}
+
+static void gdrom_fifo_writew(const void *buf, unsigned int words)
+{
+       iowrite16_rep((void __iomem *)GDROM_DATA_REG, buf, words);
+}
+
 static void gdrom_spicommand(void *spi_string, int buflen)
 {
        short *cmd = spi_string;

Quoted 42 lines · Florian Fuchs

GDROM_DATA_REG is a memory-mapped data register, but the driver uses outsw() and insw() only for this register. Replace this with local helpers using MMIO accessors ioread16_rep() / iowrite16_rep().

Before, it oopsed accessing the data register, as the io_port_base P2SEG gets added to the argument in outsw() / insw(), which leads to an unusable drive:

        BUG: unable to handle kernel paging request at 405f7080
        PC: [<8c28d5b4>] gdrom_spicommand+0x6c/0xb0

Signed-off-by Florian Fuchs <[email protected]>

---
The original Oops can be reproduced just by mounting a disc, like:
mount -t iso9660 -o ro /dev/gdrom /mnt

---
 drivers/cdrom/gdrom.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

drivers/cdrom/gdrom.c +10 -0

index 4ba4dd06cbf4..dccf41fa5d0a 100644
@@ -171,6 +171,16 @@ static void gdrom_identifydevice(void *buf)
                data[c] = __raw_readw(GDROM_DATA_REG);
 }
 
+static void gdrom_fifo_readw(void *buf, unsigned int words)
+{
+       ioread16_rep((void __iomem *)GDROM_DATA_REG, buf, words);
+}
+
+static void gdrom_fifo_writew(const void *buf, unsigned int words)
+{
+       iowrite16_rep((void __iomem *)GDROM_DATA_REG, buf, words);
+}
+
 static void gdrom_spicommand(void *spi_string, int buflen)
 {
        short *cmd = spi_string;

drivers/cdrom/gdrom.c static void gdrom_spicommand(void *spi_string, int buflen) · line 213

Quoted 7 lines

drivers/cdrom/gdrom.c +1 -1

@@ -198,7 +208,7 @@ static void gdrom_spicommand(void *spi_string, int buflen)
                gdrom_getsense(NULL);
                return;
        }
-       outsw(GDROM_DATA_REG, cmd, 6);
+       gdrom_fifo_writew(cmd, 6);
 }
 

This is one of those "how did this ever work to begin with" bugs when examined today - bur rather than introduce new local functions can we not just use either readsw(p, d, l)/writesw(p, d, l) or __raw_writew(p,d,l) and __raw_read(p,d,l) directly?

Adrian

Update the gendisk capacity of the media. Without the capacity, the block reads fail before reaching the request queue, which prevented ISO9660 mounts. Refresh the capacity from the TOC leadout in gdrom_bdops_open() so it checks the inserted media.

Quoted 6 lines · Florian Fuchs

Update the gendisk capacity of the media. Without the capacity, the block reads fail before reaching the request queue, which prevented ISO9660 mounts. Refresh the capacity from the TOC leadout in gdrom_bdops_open() so it checks the inserted media.

...

Quoted 6 lines

drivers/cdrom/gdrom.c +1 -0

+
 static int gdrom_bdops_open(struct gendisk *disk, blk_mode_t mode)
 {
        int ret;

@@ -492,6 +511,8 @@ static int gdrom_bdops_open(struct gendisk *disk, blk_mode_t mode)
 

drivers/cdrom/gdrom.c static int gdrom_bdops_open(struct gendisk *disk, blk_mode_t mode) · line 518

Quoted 7 lines

drivers/cdrom/gdrom.c +2 -0

        mutex_lock(&gdrom_mutex);
        ret = cdrom_open(gd.cd_info, mode);
+       if (!ret)
+               ret = gdrom_update_capacity();
        mutex_unlock(&gdrom_mutex);
        return ret;
 }

Will this not reintrocue the potential race condition bug that was eliminated here -
https://github.com/mcmenaminadrian/linux/commit/2bbea6e117357d17842114c65e9a9cf2d13ae8a3
(not by me, I add)?

You mean, because one of the functions in the call tree of gdrom_update_capacity() wants to get the mutex gdrom_mutex or s_umount? I checked the locally called functions, but don't see that one tries to aquire a lock.

I don't understand the exact potential of the race condition, maybe you can elaborate, where I should look.

Florian,

Quoted 6 lines · Florian Fuchs

You mean, because one of the functions in the call tree of gdrom_update_capacity() wants to get the mutex gdrom_mutex or s_umount? I checked the locally called functions, but don't see that one tries to aquire a lock.

Quoted 3 lines

I don't understand the exact potential of the race condition, maybe you can elaborate, where I should look.

Actually, having looked at the code path in detail too I agree that this won't recreate the problem the earlier patch addressed, so this looks good to me.

Quoted 12 lines · Adrian McMenamin

Florian,

Quoted 6 lines · Florian Fuchs

You mean, because one of the functions in the call tree of gdrom_update_capacity() wants to get the mutex gdrom_mutex or s_umount? I checked the locally called functions, but don't see that one tries to aquire a lock.

Quoted 8 lines

Quoted 3 lines

I don't understand the exact potential of the race condition, maybe you can elaborate, where I should look.

Actually, having looked at the code path in detail too I agree that this won't recreate the problem the earlier patch addressed, so this looks good to me.

Sorry - a further thought. This rechecking for capacity is only truly necessary if there has been a change of medium - is that right? So we can take the result of the earlier disk_check_media_change and if that returns true update the capacity? Is that not correct?

Hi all,

This series fixes a gdrom driver Oops due to bad MMIO register access and fixes the missing updates of the block layer gendisk capacity that prevented ISO9660 mounts from working.

The change was tested on real Sega Dreamcast devices (PAL-E, NTSC-J) with physical CD-R discs and with GDEMU emulated discs. Before: Oops on mount and an unusable drive. After: Successfully able to mount and use the inserted medium.

Thanks,
Florian

Florian Fuchs (2):
cdrom: gdrom: replace port I/O with MMIO accessors
cdrom: gdrom: update gendisk capacity on open

 drivers/cdrom/gdrom.c | 39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f

Hi Florian,

Quoted 22 lines · Florian Fuchs

Hi all,

This series fixes a gdrom driver Oops due to bad MMIO register access and fixes the missing updates of the block layer gendisk capacity that prevented ISO9660 mounts from working.

The change was tested on real Sega Dreamcast devices (PAL-E, NTSC-J) with physical CD-R discs and with GDEMU emulated discs. Before: Oops on mount and an unusable drive. After: Successfully able to mount and use the inserted medium.

Thanks,
Florian

Florian Fuchs (2):
cdrom: gdrom: replace port I/O with MMIO accessors
cdrom: gdrom: update gendisk capacity on open

 drivers/cdrom/gdrom.c | 39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

Quoted 1 line

base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f

Thanks a lot for the series! Can you give me any hints on what's the best method these days to boot a Linux kernel on Dreamcast these days? I have the hardware, but I never tried to boot Linux on it. An emulator might be even better.

Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913