This patch adds support for the CD-Rom drive on the SEGA Dreamcast.
The SEGA Dreamcast has a built in CD-Rom drive, electrically similar
to an ATA-3 drive, but implementing a proprietary packet interface -
the so-called Sega Packet Interface (SPI)- and also supporting a
proprietary format of disk - the Giga Disk Rom, with a 1GB capacity.
The drive is know as the "GD-Rom drive".
This patch partially implements the SPI and also supports reading GD
Rom disks. Unlike previous GD Rom drivers (which were never in the
mainline), this driver implements DMA and not PIO for reads. It is a
new driver, not a modified version of previous drivers.
This is the fourth iteration of this patch - which should work with
both 2.6-24-rc5 and Paul Mundt's 2.6.25 queue.
Signed-off by: Adrian McMenamin <[email protected]>
----
On Thu, Dec 27, 2007 at 01:26:47AM +0000, Adrian McMenamin wrote:
> This patch adds support for the CD-Rom drive on the SEGA Dreamcast.
>
Please fix your mailer to inline the patch, preferably without word
wrapping. This is not a difficult thing, send yourself some test mails
until you get it sorted out.
> diff -rupN linux-2.6-orig/drivers/block/Kconfig linux-2.6/drivers/block/Kconfig
> --- linux-2.6-orig/drivers/block/Kconfig 2007-12-26 17:27:14.000000000 +0000
> +++ linux-2.6/drivers/block/Kconfig 2007-12-27 00:08:39.000000000 +0000
> @@ -105,6 +105,18 @@ config PARIDE
> "MicroSolutions backpack protocol", "DataStor Commuter protocol"
> etc.).
>
> +config GDROM
Why is this using a tab?
> + tristate "SEGA Dreamcast GD-ROM drive"
> + depends on SH_DREAMCAST
> + help
> + A standard SEGA Dreamcast comes with a modified CD ROM drive called a
> + "GD-ROM" by SEGA to signify it is capable of reading special disks
> + with up to 1 GB of data. This drive will also read standard CD ROM
> + disks. Select this option to access any disks in your GD ROM drive.
> + Most users will want to say "Y" here.
> + You can also build this as a module which will be called gdrom.ko
> +
> +
Superfluous whitespace.
> +static int gdrom_get_last_session(struct cdrom_device_info *cd_info, struct cdrom_multisession *ms_info)
> +{
> + int fentry, lentry, track, data, tocuse, err;
> + kfree(gd.toc);
> + gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
Er, what? The size of this never changes, allocate it once, and just
overload it every time you step in to this function. There's no reason to
free it and reallocate every time. Shove it in your probe routine with
the rest of your kzallocs.
> +/* keep the function loioking like the universal CD Rom specification - returning int*/
looking.
> +static int gdrom_set_command_interrupt_handler(void)
> +{
> + /* need a queue to wait in */
> + init_waitqueue_head(&command_queue);
> + return request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt, IRQF_DISABLED, "gdrom_command", &gd);
> +}
> +
> +static int gdrom_set_dma_interrupt_handler(void)
> +{
> + init_waitqueue_head(&request_queue);
> + return request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt, IRQF_DISABLED, "gdrom_dma", &gd);
> +}
> +
Having separate functions for these is pretty pointless.
> + spin_lock(&gdrom_lock);
> + list_for_each_safe(elem, next, &gdrom_deferred) {
> + req = list_entry(elem, struct request, queuelist);
> + spin_unlock(&gdrom_lock);
[snip]
> + /* now seek to take the request spinlock
> + * before handling ending the request */
> + spin_lock(&gdrom_lock);
> + list_del_init(&req->queuelist);
> + blk_requeue_request(gd.gdrom_rq, req);
> + if (err)
> + end_request(req, 0);
> + else
> + end_request(req, 1);
> + }
> + spin_unlock(&gdrom_lock);
> + kfree(read_command);
> +}
> +
This locking is all over the place. What is this lock supposed to be
accomplishing?
On Thu, 2007-12-27 at 17:18 +0900, Paul Mundt wrote:
> On Thu, Dec 27, 2007 at 01:26:47AM +0000, Adrian McMenamin wrote:
>
> > + /* now seek to take the request spinlock
> > + * before handling ending the request */
> > + spin_lock(&gdrom_lock);
> > + list_del_init(&req->queuelist);
> > + blk_requeue_request(gd.gdrom_rq, req);
> > + if (err)
> > + end_request(req, 0);
> > + else
> > + end_request(req, 1);
> > + }
> > + spin_unlock(&gdrom_lock);
> > + kfree(read_command);
> > +}
> > +
> This locking is all over the place. What is this lock supposed to be
> accomplishing?
> -
I have to hold the lock to access the request queue. As the linked list
of deferred requests is under the control of code also protected by the
lock, it is also held to ensure manipulation of that list is serialised.
The first step of the loop manipulates that linked list - so it is held
as we re-iterate over the loop.
This is pretty much the way Jens recommended I do it.
On Thursday 27 December 2007, Paul Mundt wrote:
> On Thu, Dec 27, 2007 at 01:26:47AM +0000, Adrian McMenamin wrote:
> > +static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
> > struct cdrom_multisession *ms_info) +{
> > + int fentry, lentry, track, data, tocuse, err;
> > + kfree(gd.toc);
> > + gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
>
> Er, what? The size of this never changes, allocate it once, and just
> overload it every time you step in to this function. There's no reason to
> free it and reallocate every time. Shove it in your probe routine with
> the rest of your kzallocs.
since gd is a global, just dont declare toc as a pointer ...
-mike
On Thu, Dec 27 2007, Adrian McMenamin wrote:
>
> On Thu, 2007-12-27 at 17:18 +0900, Paul Mundt wrote:
> > On Thu, Dec 27, 2007 at 01:26:47AM +0000, Adrian McMenamin wrote:
>
> >
> > > + /* now seek to take the request spinlock
> > > + * before handling ending the request */
> > > + spin_lock(&gdrom_lock);
> > > + list_del_init(&req->queuelist);
> > > + blk_requeue_request(gd.gdrom_rq, req);
> > > + if (err)
> > > + end_request(req, 0);
> > > + else
> > > + end_request(req, 1);
> > > + }
> > > + spin_unlock(&gdrom_lock);
> > > + kfree(read_command);
> > > +}
> > > +
> > This locking is all over the place. What is this lock supposed to be
> > accomplishing?
> > -
>
> I have to hold the lock to access the request queue. As the linked list
> of deferred requests is under the control of code also protected by the
> lock, it is also held to ensure manipulation of that list is serialised.
>
> The first step of the loop manipulates that linked list - so it is held
> as we re-iterate over the loop.
>
> This is pretty much the way Jens recommended I do it.
I didn't recommend the last requeue bit, it looks like a work-around due
to the way that end_request() works. The kerneldoc comment for that
function also tells you NOT to use this in new code. Use
end_dequeued_request() and get rid of the requeue, and streamline 'err'
so you can just pass it directly in.
The locking otherwise looks fine to me.
--
Jens Axboe