lkml.org 
[lkml]   [2007]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: mcdx -- do_request(): non-read command to cd!!
    On Mon, Apr 02 2007, Boaz Harrosh wrote:
    > Pekka J Enberg wrote:
    > > On Mon, 2 Apr 2007, Jens Axboe wrote:
    > >> But as I wrote earlier, it'll take lots more to make this driver close
    > >> to functional.
    > >
    > > Heh, looking at the driver, that's quite an understatement =). Rene,
    > > here's an untested attempt to use a mutex instead of the horribly broken
    > > waitqueue "synchronization" in mcdx. It may or may not help so give it a
    > > spin if you want.
    > >
    > > Pekka
    > >
    > > ---
    > > drivers/cdrom/mcdx.c | 121 ++++++++++++++++++---------------------------------
    > > 1 file changed, 44 insertions(+), 77 deletions(-)
    > >
    > > Index: 2.6/drivers/cdrom/mcdx.c
    > > ===================================================================
    > > --- 2.6.orig/drivers/cdrom/mcdx.c 2007-04-02 11:50:40.000000000 +0300
    > > +++ 2.6/drivers/cdrom/mcdx.c 2007-04-02 11:51:20.000000000 +0300
    > > @@ -58,6 +58,7 @@ = "$Id: mcdx.c,v 1.21 1997/01/26 07:
    > >
    > > #include <linux/module.h>
    > >
    > > +#include <linux/delay.h>
    > > #include <linux/errno.h>
    > > #include <linux/interrupt.h>
    > > #include <linux/fs.h>
    > > @@ -151,15 +152,14 @@ struct s_version {
    > > /* Per drive/controller stuff **************************************/
    > >
    > > struct s_drive_stuff {
    > > + struct mutex mutex;
    > > +
    > > /* waitqueues */
    > > wait_queue_head_t busyq;
    > > - wait_queue_head_t lockq;
    > > - wait_queue_head_t sleepq;
    > >
    > > /* flags */
    > > volatile int introk; /* status of last irq operation */
    > > volatile int busy; /* drive performs an operation */
    > > - volatile int lock; /* exclusive usage */
    > >
    > > /* cd infos */
    > > struct s_diskinfo di;
    > > @@ -266,7 +266,6 @@ static unsigned int uint2bcd(unsigned in
    > > static unsigned int bcd2uint(unsigned char);
    > > static unsigned port(int *);
    > > static int irq(int *);
    > > -static void mcdx_delay(struct s_drive_stuff *, long jifs);
    > > static int mcdx_transfer(struct s_drive_stuff *, char *buf, int sector,
    > > int nr_sectors);
    > > static int mcdx_xfer(struct s_drive_stuff *, char *buf, int sector,
    > > @@ -287,7 +286,7 @@ static int mcdx_requestmultidiskinfo(str
    > > static int mcdx_requesttocdata(struct s_drive_stuff *, struct s_diskinfo *,
    > > int);
    > > static int mcdx_getstatus(struct s_drive_stuff *, int);
    > > -static int mcdx_getval(struct s_drive_stuff *, int to, int delay, char *);
    > > +static int mcdx_getval(struct s_drive_stuff *, int to, int delay_sec, char *);
    > > static int mcdx_talk(struct s_drive_stuff *,
    > > const unsigned char *cmd, size_t,
    > > void *buffer, size_t size, unsigned int timeout, int);
    > > @@ -577,56 +576,57 @@ static void do_mcdx_request(request_queu
    > > if (!req)
    > > return;
    > >
    > > + if (!blk_fs_request(req)) {
    > > + end_request(req, 0);
    > > + goto again;
    > > + }
    > > +
    > > stuffp = req->rq_disk->private_data;
    > >
    > > if (!stuffp->present) {
    > > xwarn("do_request(): bad device: %s\n",req->rq_disk->disk_name);
    > > xtrace(REQUEST, "end_request(0): bad device\n");
    > > end_request(req, 0);
    > > - return;
    > > + goto again;
    > > }
    > >
    > > if (stuffp->audio) {
    > > xwarn("do_request() attempt to read from audio cd\n");
    > > xtrace(REQUEST, "end_request(0): read from audio\n");
    > > end_request(req, 0);
    > > - return;
    > > + goto again;
    > > }
    > >
    > > xtrace(REQUEST, "do_request() (%lu + %lu)\n",
    > > req->sector, req->nr_sectors);
    > >
    > > - if (req->cmd != READ) {
    > > + if (rq_data_dir(req) != READ) {
    > > xwarn("do_request(): non-read command to cd!!\n");
    > > xtrace(REQUEST, "end_request(0): write\n");
    > > end_request(req, 0);
    > > - return;
    > > - }
    > > - else {
    > > - stuffp->status = 0;
    > > - while (req->nr_sectors) {
    > > - int i;
    > > -
    > > - i = mcdx_transfer(stuffp,
    > > - req->buffer,
    > > - req->sector,
    > > - req->nr_sectors);
    > > -
    > > - if (i == -1) {
    > > - end_request(req, 0);
    > > - goto again;
    > > - }
    > > - req->sector += i;
    > > - req->nr_sectors -= i;
    > > - req->buffer += (i * 512);
    > > - }
    > > - end_request(req, 1);
    > > goto again;
    > > -
    > > - xtrace(REQUEST, "end_request(1)\n");
    > > - end_request(req, 1);
    > > }
    > >
    > > + stuffp->status = 0;
    > > + while (req->nr_sectors) {
    > > + int i;
    > > +
    > > + spin_unlock_irq(q->queue_lock);
    > > + i = mcdx_transfer(stuffp,
    > > + req->buffer,
    > > + req->sector,
    > > + req->nr_sectors);
    > > + spin_lock_irq(q->queue_lock);
    > > +
    > > + if (i == -1) {
    > > + end_request(req, 0);
    > > + goto again;
    > > + }
    > > + req->sector += i;
    > > + req->nr_sectors -= i;
    > > + req->buffer += (i * 512);
    > > + }
    > > + end_request(req, 1);
    > > goto again;
    > > }
    > >
    > > @@ -825,26 +825,6 @@ __setup("mcdx=", mcdx_setup);
    > >
    > > /* DIRTY PART ******************************************************/
    > >
    > > -static void mcdx_delay(struct s_drive_stuff *stuff, long jifs)
    > > -/* This routine is used for sleeping.
    > > - * A jifs value <0 means NO sleeping,
    > > - * =0 means minimal sleeping (let the kernel
    > > - * run for other processes)
    > > - * >0 means at least sleep for that amount.
    > > - * May be we could use a simple count loop w/ jumps to itself, but
    > > - * I wanna make this independent of cpu speed. [1 jiffy is 1/HZ] sec */
    > > -{
    > > - if (jifs < 0)
    > > - return;
    > > -
    > > - xtrace(SLEEP, "*** delay: sleepq\n");
    > > - interruptible_sleep_on_timeout(&stuff->sleepq, jifs);
    > > - xtrace(SLEEP, "delay awoken\n");
    > > - if (signal_pending(current)) {
    > > - xtrace(SLEEP, "got signal\n");
    > > - }
    > > -}
    > > -
    > > static irqreturn_t mcdx_intr(int irq, void *dev_id)
    > > {
    > > struct s_drive_stuff *stuffp = dev_id;
    > > @@ -902,13 +882,7 @@ static int mcdx_talk(struct s_drive_stuf
    > > if ((discard = (buffer == NULL)))
    > > buffer = &c;
    > >
    > > - while (stuffp->lock) {
    > > - xtrace(SLEEP, "*** talk: lockq\n");
    > > - interruptible_sleep_on(&stuffp->lockq);
    > > - xtrace(SLEEP, "talk: awoken\n");
    > > - }
    > > -
    > > - stuffp->lock = 1;
    > > + mutex_lock(&stuffp->mutex);
    > >
    > > /* An operation other then reading data destroys the
    > > * data already requested and remembered in stuffp->request, ... */
    > > @@ -992,8 +966,7 @@ xtrace(TALK, "talk() got 0x%02x\n", *
    > > xinfo("talk() giving up\n");
    > > #endif
    > >
    > > - stuffp->lock = 0;
    > > - wake_up_interruptible(&stuffp->lockq);
    > > + mutex_unlock(&stuffp->mutex);
    > >
    > > xtrace(TALK, "talk() done with 0x%02x\n", st);
    > > return st;
    > > @@ -1106,9 +1079,9 @@ stuffp->present = 0; /* this should be
    > > stuffp->wreg_hcon = stuffp->wreg_reset + 1;
    > > stuffp->wreg_chn = stuffp->wreg_hcon + 1;
    > >
    > > + mutex_init(&stuffp->mutex);
    > > +
    > > init_waitqueue_head(&stuffp->busyq);
    > > - init_waitqueue_head(&stuffp->lockq);
    > > - init_waitqueue_head(&stuffp->sleepq);
    > >
    > > /* check if i/o addresses are available */
    > > if (!request_region(stuffp->wreg_data, MCDX_IO_SIZE, "mcdx")) {
    > > @@ -1203,7 +1176,7 @@ return 0;
    > > xtrace(INIT, "init() get garbage\n");
    > > {
    > > int i;
    > > - mcdx_delay(stuffp, HZ / 2);
    > > + msleep_interruptible(500);
    > > for (i = 100; i; i--)
    > > (void) inb(stuffp->rreg_status);
    > > }
    > > @@ -1327,10 +1300,6 @@ int done = 0;
    > > return -1;
    > > }
    > >
    > > - while (stuffp->lock) {
    > > - interruptible_sleep_on(&stuffp->lockq);
    > > - }
    > > -
    > > if (stuffp->valid && (sector >= stuffp->pending)
    > > && (sector < stuffp->low_border)) {
    > >
    > > @@ -1346,7 +1315,7 @@ int done = 0;
    > > sector + nr_sectors)
    > > ? stuffp->high_border : border;
    > >
    > > - stuffp->lock = current->pid;
    > > + mutex_lock(&stuffp->mutex);
    > >
    > > do {
    > >
    > > @@ -1366,11 +1335,11 @@ int done = 0;
    > > } else
    > > continue;
    > >
    > > - stuffp->lock = 0;
    > > + mutex_unlock(&stuffp->mutex);
    > > +
    > > stuffp->busy = 0;
    > > stuffp->valid = 0;
    > >
    > > - wake_up_interruptible(&stuffp->lockq);
    > > xtrace(XFER, "transfer() done (-1)\n");
    > > return -1;
    > > }
    > > @@ -1405,9 +1374,7 @@ insb(stuffp->rreg_data, &dummy[0], C
    > > }
    > > } while (++(stuffp->pending) < border);
    > >
    > > - stuffp->lock = 0;
    > > - wake_up_interruptible(&stuffp->lockq);
    > > -
    > > + mutex_unlock(&stuffp->mutex);
    > > } else {
    > >
    > > /* The requested sector(s) is/are out of the
    > > @@ -1654,7 +1621,7 @@ cmd[0], cmd[1], cmd[2], cmd[3],
    > >
    > > outsb(stuffp->wreg_data, cmd, sizeof cmd);
    > >
    > > - if (-1 == mcdx_getval(stuffp, 3 * HZ, 0, NULL)) {
    > > + if (-1 == mcdx_getval(stuffp, 3, 0, NULL)) {
    > > xwarn("playmsf() timeout\n");
    > > return -1;
    > > }
    > > @@ -1907,7 +1874,7 @@ return mcdx_talk(stuffp, "\x40", 1, NUL
    > > }
    > >
    > > static int
    > > -mcdx_getval(struct s_drive_stuff *stuffp, int to, int delay, char *buf)
    > > +mcdx_getval(struct s_drive_stuff *stuffp, int to, int delay_sec, char *buf)
    > > {
    > > unsigned long timeout = to + jiffies;
    > > char c;
    > > @@ -1918,7 +1885,7 @@ mcdx_getval(struct s_drive_stuff *stuffp
    > > while (inb(stuffp->rreg_status) & MCDX_RBIT_STEN) {
    > > if (time_after(jiffies, timeout))
    > > return -1;
    > > - mcdx_delay(stuffp, delay);
    > > + msleep_interruptible(delay_sec * 1000);
    > > }
    > >
    > > *buf = (unsigned char) inb(stuffp->rreg_data) & 0xff;
    > > -
    > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > > the body of a message to majordomo@vger.kernel.org
    > > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > > Please read the FAQ at http://www.tux.org/lkml/
    >
    > NACK
    >
    > Why are you using req->buffer in new code. req->buffer is a leftover from a
    > very old block pc era and is supposed to be killed. Please do not use it in
    > any new code.
    > you should use bio_data(rq->bio) and if you must have a virtual memory pointer
    > hanging around than keep it in private space.

    While that is true, this is ancient crap code and there's not much point
    in rewriting that part as well. I somehow doubt that the cards are
    capable of highmem addressing in the first place :-)

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-04-02 11:47    [W:0.045 / U:1.948 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site