On Tue, Nov 04 2003, Linux Kernel Mailing List wrote:
> ChangeSet 1.1413, 2003/11/04 08:01:30-08:00, [email protected]
>
> [PATCH] fix rq->flags use in ide-tape.c
>
> Noticed by [email protected]:
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
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,
> > [email protected]
> >
> > [PATCH] fix rq->flags use in ide-tape.c
> >
> > Noticed by [email protected]:
>
> 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)
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!!!
> 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 ?
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,
> > > [email protected]
> > >
> > > [PATCH] fix rq->flags use in ide-tape.c
> > >
> > > Noticed by [email protected]:
> >
> > 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
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,
> > > [email protected]
> > >
> > > [PATCH] fix rq->flags use in ide-tape.c
> > >
> > > Noticed by [email protected]:
> >
> > 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
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
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,
> > > > [email protected]
> > > >
> > > > [PATCH] fix rq->flags use in ide-tape.c
> > > >
> > > > Noticed by [email protected]:
> > >
> > > 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
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,
> > > > > [email protected]
> > > > >
> > > > > [PATCH] fix rq->flags use in ide-tape.c
> > > > >
> > > > > Noticed by [email protected]:
> > > >
> > > > 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
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,
> > > > > > [email protected]
> > > > > >
> > > > > > [PATCH] fix rq->flags use in ide-tape.c
> > > > > >
> > > > > > Noticed by [email protected]:
> > > > >
> > > > > 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