Re: [PATCH] fix rq->flags use in ide-tape.c

Re: [PATCH] fix rq->flags use in ide-tape.c

[Jens Axboe]

On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> ChangeSet 1.1413, 2003/11/04 08:01:30-08:00, B.Zolnierkiewicz@elka.pw.edu.pl
> 
> 	[PATCH] fix rq->flags use in ide-tape.c
> 	
> 	Noticed by Stuart_Hayes@Dell.com:

Guys, this is _way_ ugly. We definitely dont need more crap in ->flags
for private driver use, stuff them somewhere else in the rq. rq->cmd[0]
usage would be a whole lot better. This patch should never have been
merged. If each and every driver needs 5 private bits in ->flags,
well...

Was this even posted on linux-kernel for review?

> @@ -218,6 +223,11 @@
>  #define REQ_PM_SUSPEND	(1 << __REQ_PM_SUSPEND)
>  #define REQ_PM_RESUME	(1 << __REQ_PM_RESUME)
>  #define REQ_PM_SHUTDOWN	(1 << __REQ_PM_SHUTDOWN)
> +#define REQ_IDETAPE_PC1 (1 << __REQ_IDETAPE_PC1)
> +#define REQ_IDETAPE_PC2 (1 << __REQ_IDETAPE_PC2)
> +#define REQ_IDETAPE_READ	(1 << __REQ_IDETAPE_READ)
> +#define REQ_IDETAPE_WRITE	(1 << __REQ_IDETAPE_WRITE)
> +#define REQ_IDETAPE_READ_BUFFER	(1 << __REQ_IDETAPE_READ_BUFFER)


-- 
Jens Axboe

Re: [PATCH] fix rq->flags use in ide-tape.c

[Bartlomiej Zolnierkiewicz]

On Wednesday 05 of November 2003 09:40, Jens Axboe wrote:
> On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> > ChangeSet 1.1413, 2003/11/04 08:01:30-08:00,
> > B.Zolnierkiewicz@elka.pw.edu.pl
> >
> > 	[PATCH] fix rq->flags use in ide-tape.c
> >
> > 	Noticed by Stuart_Hayes@Dell.com:
>
> Guys, this is _way_ ugly. We definitely dont need more crap in ->flags
> for private driver use, stuff them somewhere else in the rq. rq->cmd[0]
> usage would be a whole lot better. This patch should never have been
> merged. If each and every driver needs 5 private bits in ->flags,
> well...

Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem in
ide-tape.c, but if you prefer this way I can clean it up.  I just wanted
minimal changes to ide-tape.c to make it working.

> Was this even posted on linux-kernel for review?

Yes.

--bartlomiej

> > @@ -218,6 +223,11 @@
> >  #define REQ_PM_SUSPEND	(1 << __REQ_PM_SUSPEND)
> >  #define REQ_PM_RESUME	(1 << __REQ_PM_RESUME)
> >  #define REQ_PM_SHUTDOWN	(1 << __REQ_PM_SHUTDOWN)
> > +#define REQ_IDETAPE_PC1 (1 << __REQ_IDETAPE_PC1)
> > +#define REQ_IDETAPE_PC2 (1 << __REQ_IDETAPE_PC2)
> > +#define REQ_IDETAPE_READ	(1 << __REQ_IDETAPE_READ)
> > +#define REQ_IDETAPE_WRITE	(1 << __REQ_IDETAPE_WRITE)
> > +#define REQ_IDETAPE_READ_BUFFER	(1 << __REQ_IDETAPE_READ_BUFFER)

Re: [PATCH] fix rq->flags use in ide-tape.c

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]


> Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem in
> ide-tape.c, but if you prefer this way I can clean it up.  I just wanted
> minimal changes to ide-tape.c to make it working.

isn't the right answer "use ide-scsi and scsi-tape" for IDE based tape
drives ?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] fix rq->flags use in ide-tape.c

[Bartlomiej Zolnierkiewicz]

On Wednesday 05 of November 2003 13:14, Arjan van de Ven wrote:
> > Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem in
> > ide-tape.c, but if you prefer this way I can clean it up.  I just wanted
> > minimal changes to ide-tape.c to make it working.
>
> isn't the right answer "use ide-scsi and scsi-tape" for IDE based tape
> drives ?

NO!!!

Re: [PATCH] fix rq->flags use in ide-tape.c

[Jens Axboe]

On Wed, Nov 05 2003, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 05 of November 2003 09:40, Jens Axboe wrote:
> > On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> > > ChangeSet 1.1413, 2003/11/04 08:01:30-08:00,
> > > B.Zolnierkiewicz@elka.pw.edu.pl
> > >
> > > 	[PATCH] fix rq->flags use in ide-tape.c
> > >
> > > 	Noticed by Stuart_Hayes@Dell.com:
> >
> > Guys, this is _way_ ugly. We definitely dont need more crap in ->flags
> > for private driver use, stuff them somewhere else in the rq. rq->cmd[0]
> > usage would be a whole lot better. This patch should never have been
> > merged. If each and every driver needs 5 private bits in ->flags,
> > well...
> 
> Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem in
> ide-tape.c, but if you prefer this way I can clean it up.  I just wanted
> minimal changes to ide-tape.c to make it working.

Well using ->cmd is acceptable. Adding these 5 bits for ide-tape is, on
the other hand, completely and utterly unacceptable. So I'd greatly
prefer it that way :)

> > Was this even posted on linux-kernel for review?
> 
> Yes.

Ok missed it then.

-- 
Jens Axboe

Re: [PATCH] fix rq->flags use in ide-tape.c

[Jens Axboe]

On Wed, Nov 05 2003, Arjan van de Ven wrote:
> 
> > Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem in
> > ide-tape.c, but if you prefer this way I can clean it up.  I just wanted
> > minimal changes to ide-tape.c to make it working.
> 
> isn't the right answer "use ide-scsi and scsi-tape" for IDE based tape
> drives ?

No comment :0

-- 
Jens Axboe

Re: [PATCH] fix rq->flags use in ide-tape.c

[Bartlomiej Zolnierkiewicz]

On Wednesday 05 of November 2003 13:00, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 05 of November 2003 09:40, Jens Axboe wrote:
> > On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> > > ChangeSet 1.1413, 2003/11/04 08:01:30-08:00,
> > > B.Zolnierkiewicz@elka.pw.edu.pl
> > >
> > > 	[PATCH] fix rq->flags use in ide-tape.c
> > >
> > > 	Noticed by Stuart_Hayes@Dell.com:
> >
> > Guys, this is _way_ ugly. We definitely dont need more crap in ->flags
> > for private driver use, stuff them somewhere else in the rq. rq->cmd[0]
> > usage would be a whole lot better. This patch should never have been
> > merged. If each and every driver needs 5 private bits in ->flags,
> > well...
>
> Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem in
> ide-tape.c, but if you prefer this way I can clean it up.  I just wanted
> minimal changes to ide-tape.c to make it working.

Also putting these flags in rq->cmd[0] makes it hard to later convert
ide-tape.c to use rq->cmd[] for storing packet commands.

--bartlomiej

Re: [PATCH] fix rq->flags use in ide-tape.c

[Jens Axboe]

On Wed, Nov 05 2003, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 05 of November 2003 13:00, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 05 of November 2003 09:40, Jens Axboe wrote:
> > > On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> > > > ChangeSet 1.1413, 2003/11/04 08:01:30-08:00,
> > > > B.Zolnierkiewicz@elka.pw.edu.pl
> > > >
> > > > 	[PATCH] fix rq->flags use in ide-tape.c
> > > >
> > > > 	Noticed by Stuart_Hayes@Dell.com:
> > >
> > > Guys, this is _way_ ugly. We definitely dont need more crap in ->flags
> > > for private driver use, stuff them somewhere else in the rq. rq->cmd[0]
> > > usage would be a whole lot better. This patch should never have been
> > > merged. If each and every driver needs 5 private bits in ->flags,
> > > well...
> >
> > Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem in
> > ide-tape.c, but if you prefer this way I can clean it up.  I just wanted
> > minimal changes to ide-tape.c to make it working.
> 
> Also putting these flags in rq->cmd[0] makes it hard to later convert
> ide-tape.c to use rq->cmd[] for storing packet commands.

What's wrong with just looking at the opcode instead of inventing magic
flags. Seems like _just_ the right thing to do, convert to really using
rq and killing the private command stuff as much as possible. The latter
can wait though, the flag thing really has to go right now.

ide-*.c driver by Gadi are all completely over designed and attempts to
basically implement everything themselves. Horrible.

-- 
Jens Axboe

Re: [PATCH] fix rq->flags use in ide-tape.c

[Bartlomiej Zolnierkiewicz]

On Wednesday 05 of November 2003 14:56, Jens Axboe wrote:
> On Wed, Nov 05 2003, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 05 of November 2003 13:00, Bartlomiej Zolnierkiewicz wrote:
> > > On Wednesday 05 of November 2003 09:40, Jens Axboe wrote:
> > > > On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> > > > > ChangeSet 1.1413, 2003/11/04 08:01:30-08:00,
> > > > > B.Zolnierkiewicz@elka.pw.edu.pl
> > > > >
> > > > > 	[PATCH] fix rq->flags use in ide-tape.c
> > > > >
> > > > > 	Noticed by Stuart_Hayes@Dell.com:
> > > >
> > > > Guys, this is _way_ ugly. We definitely dont need more crap in
> > > > ->flags for private driver use, stuff them somewhere else in the rq.
> > > > rq->cmd[0] usage would be a whole lot better. This patch should never
> > > > have been merged. If each and every driver needs 5 private bits in
> > > > ->flags, well...
> > >
> > > Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem
> > > in ide-tape.c, but if you prefer this way I can clean it up.  I just
> > > wanted minimal changes to ide-tape.c to make it working.
> >
> > Also putting these flags in rq->cmd[0] makes it hard to later convert
> > ide-tape.c to use rq->cmd[] for storing packet commands.
>
> What's wrong with just looking at the opcode instead of inventing magic
> flags. Seems like _just_ the right thing to do, convert to really using
> rq and killing the private command stuff as much as possible. The latter
> can wait though, the flag thing really has to go right now.

It is non-trivial cause it seems packet commands are prepared during
processing request, not prior to queuing it.  Also I still don't fully
understand driver inner-workings with respect to DSC, "pipeline" and internal
commands processing.

> ide-*.c driver by Gadi are all completely over designed and attempts to
> basically implement everything themselves. Horrible.

Yep, but we should be careful in removing cruft.  I've already had a hard time
fixing it after (totally unnecessary and broken) buffer_head->bio conversion.

cheers,
--bartlomiej

Re: [PATCH] fix rq->flags use in ide-tape.c

[Jens Axboe]

On Wed, Nov 05 2003, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 05 of November 2003 14:56, Jens Axboe wrote:
> > On Wed, Nov 05 2003, Bartlomiej Zolnierkiewicz wrote:
> > > On Wednesday 05 of November 2003 13:00, Bartlomiej Zolnierkiewicz wrote:
> > > > On Wednesday 05 of November 2003 09:40, Jens Axboe wrote:
> > > > > On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> > > > > > ChangeSet 1.1413, 2003/11/04 08:01:30-08:00,
> > > > > > B.Zolnierkiewicz@elka.pw.edu.pl
> > > > > >
> > > > > > 	[PATCH] fix rq->flags use in ide-tape.c
> > > > > >
> > > > > > 	Noticed by Stuart_Hayes@Dell.com:
> > > > >
> > > > > Guys, this is _way_ ugly. We definitely dont need more crap in
> > > > > ->flags for private driver use, stuff them somewhere else in the rq.
> > > > > rq->cmd[0] usage would be a whole lot better. This patch should never
> > > > > have been merged. If each and every driver needs 5 private bits in
> > > > > ->flags, well...
> > > >
> > > > Yeah, it is ugly.  Using rq->cmd is also ugly as it hides the problem
> > > > in ide-tape.c, but if you prefer this way I can clean it up.  I just
> > > > wanted minimal changes to ide-tape.c to make it working.
> > >
> > > Also putting these flags in rq->cmd[0] makes it hard to later convert
> > > ide-tape.c to use rq->cmd[] for storing packet commands.
> >
> > What's wrong with just looking at the opcode instead of inventing magic
> > flags. Seems like _just_ the right thing to do, convert to really using
> > rq and killing the private command stuff as much as possible. The latter
> > can wait though, the flag thing really has to go right now.
> 
> It is non-trivial cause it seems packet commands are prepared during
> processing request, not prior to queuing it.  Also I still don't fully

Definitely, it can be done at a later stage.

> understand driver inner-workings with respect to DSC, "pipeline" and internal
> commands processing.

I don't blame you :)

> > ide-*.c driver by Gadi are all completely over designed and attempts to
> > basically implement everything themselves. Horrible.
> 
> Yep, but we should be careful in removing cruft.  I've already had a
> hard time fixing it after (totally unnecessary and broken)
> buffer_head->bio conversion.

Yup that wasn't a very good conversion... I think I even complained at
the time to whomever did it. In my opinion, it's better to keep the
driver broken than accept a bad conversion (someone doing it without
really understanding the driver nor bio stuff) that makes it compile.
The latter is what happened.

-- 
Jens Axboe