Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast

Jens Axboe
2007-12-17T14:10:47Z

On Sun, Dec 16 2007, Adrian McMenamin wrote:
> +static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
> +{
> + int err;
> + struct packet_command *read_command;
> + /* release the spin lock but check later
> + * we're not in the middle of some dma */
> + spin_unlock(&gdrom_lock);
> + ctrl_outl(0x8843407F, GDROM_DMA_ACCESS_CTRL_REG); /* memory setting */
> + ctrl_outl(9, GDROM_DMA_WAIT_REG); /* DMA word setting */
> + ctrl_outl(PHYSADDR(buffer), GDROM_DMA_STARTADDR_REG);
> + ctrl_outl(block_cnt * GDROM_HARD_SECTOR, GDROM_DMA_LENGTH_REG);
> + ctrl_outl(1, GDROM_DMA_DIRECTION_REG);
> + ctrl_outl(1, GDROM_DMA_ENABLE_REG);
> + /* send command */
> + read_command = kzalloc(sizeof(struct packet_command), GFP_KERNEL);
> + if (!read_command)
> + return -ENOMEM;
> + read_command->cmd[0] = 0x30;
> + read_command->cmd[1] = 0x20;
> + read_command->cmd[2] = (block >> 16) & 0xFF;
> + read_command->cmd[3] = (block >> 8) & 0xFF;
> + read_command->cmd[4] = block & 0xFF;
> + read_command->cmd[8] = (block_cnt >> 16) & 0xFF;
> + read_command->cmd[9] = (block_cnt >> 8) & 0xFF;
> + read_command->cmd[10] = block_cnt & 0xFF;
> + /* set for DMA */
> + ctrl_outb(1, GDROM_ERROR_REG);
> + /* other parameters */
> + ctrl_outb(0, GDROM_SECNUM_REG);
> + ctrl_outb(0, GDROM_BCL_REG);
> + ctrl_outb(0, GDROM_BCH_REG);
> + ctrl_outb(0, GDROM_DSEL_REG);
> + ctrl_outb(0, GDROM_INTSEC_REG);
> + /* In multiple DMA transfers need to wait */
> + while (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80)
> + cpu_relax();
> + ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
> + while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8)
> + cpu_relax(); /* wait for DRQ to be set to 1 */
> + gd.pending = 1;
> + gd.transfer = 1;
> + outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
> + while (ctrl_inb(GDROM_DMA_STATUS_REG))
> + cpu_relax();
> + ctrl_outb(1, GDROM_DMA_STATUS_REG);
> + /* 5 second error margin here seems more reasonable */
> + wait_event_interruptible_timeout(request_queue, gd.transfer == 0, HZ * 5);
> + err = ctrl_inb(GDROM_DMA_STATUS_REG);
> + err = gd.transfer;
> + gd.transfer = 0;
> + gd.pending = 0;
> + kfree(read_command);
> + spin_lock(&gdrom_lock);
> + return err;
> +}
> +
> +static void gdrom_request_handler_dma(struct request *req)
> +{
> + int err, block, block_cnt;
> + err = 0;
> + block = req->sector/GD_TO_BLK + GD_SESSION_OFFSET;
> + block_cnt = req->nr_sectors/GD_TO_BLK;
> + err = gdrom_readdisk_dma(block, block_cnt, req->buffer);
> + if (unlikely(err)) {
> + end_request(req, 0);
> + spin_unlock(&gdrom_lock);
> + gdrom_getsense(NULL);
> + spin_lock(&gdrom_lock);
> + }
> + else
> + end_request(req, 1);
> +}
> +
> +static void gdrom_request(struct request_queue *rq)
> +{
> + struct request *req;
> + unsigned long pages;
> + pages = rq->backing_dev_info.ra_pages;
> + while ((req = elv_next_request(rq)) != NULL) {
> + if (! blk_fs_request(req)) {
> + printk(KERN_DEBUG "GDROM: Non-fs request ignored\n");
> + end_request(req, 0);
> + }
> + if (rq_data_dir(req)) {
> + printk(KERN_NOTICE "GDROM: Read only device - write request ignored\n");
> + end_request(req, 0);
> + }
> + if (req->nr_sectors) {
> + gdrom_request_handler_dma(req);
> + }
> + }
> +}
> +

Few notes:

- Compare rq_data_dir() with WRITE, don't just assume that any non-zero
will be a write.

- You need to offload this request handling to a workqueue of some sort,
your current request handling is very broken for two reasons: One is
that interrupts are still disabled when you drop your queue lock, so
you cannot use sleeping functions like GFP_KERNEL allocations or
wait_event(). The other is that it's illegal to sleep from your
request_fn context in the first place, since you could be stalling
others.

- You also seem to be busy waiting for other transactions to finish. Any
idea how long those might take? Perhaps put an upper bound on this
waiting, and/or do blocking waits?

- I'm assuming this hardware can't do sg transfers?

--
Jens Axboe