Message-ID: <20081217072543.GA828@gollum.tnic>
Date:	Wed, 17 Dec 2008 08:25:43 +0100
From:	Borislav Petkov <petkovbb@...glemail.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org
Subject: Re: [PATCH 0/9] ide-atapi: remove ide_atapi_pc from the irq handler

Hi,

> Hmm.  I recall discussing this before and having some concerns about struct
> ide_atapi_pc removal idea probably being premature...
> 
> Looking at the patchset itself -- I'm very sorry but I just cannot apply it
> with all these command specific fields going into ide_drive_t:
> 
>         unsigned long pc_flags;
>         int pc_req_xfer;
>         int pc_buf_size;
>         int pc_error;
> 
> While they may look similar to the existing pc_* fields:
> 
>         /* callback for packet commands */
>         void (*pc_callback)(struct ide_drive_s *, int);
> 
>         void (*pc_update_buffers)(struct ide_drive_s *, struct ide_atapi_pc *);
>         int  (*pc_io_buffers)(struct ide_drive_s *, struct ide_atapi_pc *,
>                               unsigned int, int);
> 
> they are clearly not.  Existing pc_* callbacks are per-device type (they
> should have been probably named drv_*) and new ones are per-command ones.

maybe I didn't express myself clear in the commit messages - those
fields are only temporary - the emphasis being on temporary - and was
going to remove them after the dust settles.

> I also don't see a way for code to differentiate between pc and failed_pc.

This happens only in once place in ide-tape and this is taken care of rather
clumsily with an additional flag.

> Why attack the "hard" problem first (struct ide_atapi_pc one) while there
> are easier ones to deal with?  It may happen that once these easier ones
> are fixed we will be able to view the "hard" one from a completely different
> perspective (because code's complexity will decrease a lot in the meantime
> it no longer will be a "hard" problem) and apply a better solution.
> 
> Besides I'm not fully convinced that struct ide_atapi_pc has to go first in
> order to port ide-cd over ide-atapi -- we can just instead port ide-cd to
> use struct ide_atapi_pc (however probably only after converting the driver
> to use sg-s for PIO transfers -- otherwise it becomes another "hard" problem)
> and then deal with struct ide_atapi_pc for all ATAPI code (be using struct
> request fields directly or by merging struct ide_atapi_pc into ide_task_t
> and always using the latter structure for ATA[PI] commands -- which is the
> option that I personally came to prefer lately)...

Here are the problems i see with ide_atapi_pc, judging by my somewhat limited
experience:

1. ide_issue_pc/ide_transfer_pc and all those other ATAPI functions have to
check the pc pointer depending on being entered from ide-cd or from the other
drivers, which means that either ide-cd is ported to use ide_atapi_pc's or we
have lots of spaghetti if/else checks.

2. But, it really seems too unnatural to have ide_atapi_pc structs representing
an ATAPI cmd, IMHO, and doing all those memcpy(rq->cmd, pc->c, BLK_MAX_CDB) and
generally mapping the atapi_pc's to an rq - I'd rather much prefer to use the
rqs which are pretty much sufficient for most tasks and have maybe a smaller
struct which is in rq->buffer or similar handling all the ATAPI specific things
like pc->req_xfer and such.

3. I also think that it is kinda not so smart to have a buffer in the atapi_pc
and move it all around. True, Linus removed some of the allocations on stack but
ide-floppy used to be one of the biggest stack users, ide-tape might still be,
haven't checked.

So, to sum up, I think that ide-cd and especially all the logic around
ide_cd_queue_pc and its users is much more cleaner, IMHO, than what
ide-floppy/tape do by saying

ide_init_pc()
ide_create_bla_cmd()
ide_queue_pc_head/tail() /* here you unnecessarily map the pc to an rq */
ide_issue_pc()

and, in most cases, ide_create_bla_cmd() is used only in one place so
it gets inlined by gcc. So, I think using private buffers in all those
get_capacity/check_status/read_tocentry and sending them down through an
rq is the cleaner solution because you do

cdrom_check_status()
ide_cd_queue_pc()

and this might become

ide_atapi_check_status()
ide_atapi_queue_pc()

and that's pretty awesome, don't you think?

p.s. I'm sure there's something else I'm forgetting.

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

Powered by blists - more mailing lists