Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Wednesday 11 February 2009, Sergei Shtylyov wrote:
> Hello.
> 
> Borislav Petkov wrote:
> 
> >>>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >>>> Subject: [PATCH] ide: remove ide_execute_pkt_cmd()
> >>>>
> >>>> * Pass command structure to ide_execute_command() and skip
> >>>>  __ide_set_handler() for ATAPI protocols.
> >>>>
> >>>> * Convert ide_issue_pc() to always use ide_execute_command()
> >>>>  and remove no longer needed ide_execute_pkt_cmd().
> >>>>
> >>>> There should be no functional changes caused by this patch.
> >>>>
> >>>> Cc: Borislav Petkov <petkovbb@gmail.com>
> >>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >>>> ---
> >>>>  drivers/ide/ide-atapi.c    |   15 +++++++--------
> >>>>  drivers/ide/ide-iops.c     |   23 ++++++-----------------
> >>>>  drivers/ide/ide-taskfile.c |    6 ++----
> >>>>  include/linux/ide.h        |    5 ++---
> >>>>  4 files changed, 17 insertions(+), 32 deletions(-)
> >>>>
> >>>> Index: b/drivers/ide/ide-atapi.c
> >>>> ===================================================================
> >>>> --- a/drivers/ide/ide-atapi.c
> >>>> +++ b/drivers/ide/ide-atapi.c
> >>>> @@ -481,6 +481,7 @@ static void ide_init_packet_cmd(struct i
> >>>>        cmd->protocol  = dma ? ATAPI_PROT_DMA : ATAPI_PROT_PIO;
> >>>>        cmd->tf_flags |= IDE_TFLAG_OUT_LBAH | IDE_TFLAG_OUT_LBAM |
> >>>>                         IDE_TFLAG_OUT_FEATURE | tf_flags;
> >>>> +       cmd->tf.command = ATA_CMD_PACKET;
> >>>>        cmd->tf.feature = dma;          /* Use PIO/DMA */
> >>>>        cmd->tf.lbam    = bcount & 0xff;
> >>>>        cmd->tf.lbah    = (bcount >> 8) & 0xff;
> >>>> @@ -626,6 +627,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t
> >>>>        unsigned int timeout;
> >>>>        u32 tf_flags;
> >>>>        u16 bcount;
> >>>> +       u8 drq_int = !!(drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT);
> >>>>
> >>>>         
> >>> How about we finally add those check macros in block layer fashion like
> >>> blk_pc_request et al and do
> >>>
> >>> #define drv_can_drq_interrupt(drive)    ((drive)->atapi_flags &
> >>> IDE_AFLAG_DRQ_INTERRUPT)
> >>>
> >>>       
> >>  I suppose it's for the devices that interrupt on packet DRQ? Then it's
> >> hardly a good name because it's not like this is some optional capability.
> >>     
> >
> > No, I was alluding to the command packet DRQ type used by the device as it is
> > put in SFF8020i, 7.1.7.1 General Configuration Word.
> >   
> 
>    I was talking about exactly the same feature. :-)
> 
> >>> or similar instead of wasting stack space?
> >>>       
> >>  It doesn't necessarily waste stack space. Haven't you heard about compiler
> >> putting local vairables into registers?
> >>     
> >
> > Yes, have you heard of unnecessary register spilling?
> >   
> 
>    No -- only about stack spilling on CPUs "caching" the top of stack in 
> their register file (like SPARC).
>    Linux runs not only on x86 and many RISCs can store several local 
> variables in the dedicated registers -- it's the part of say MIPS ABIs...
> 
> >>> It'll also read better in the if() check:
> >>>
> >>> if (drv_can_irq_interrupt(drive)) { ...
> >>>
> >>>       
> >>  It's faster to checj a local variable than to dereference drv several times
> >> -- unless gcc optimizes that away (by creating an implicit local variable
> >> :-).
> >>     
> >
> > I hope gcc is smart enough to do that.
> >   
> 
>    Then where's the win?

In having shorter & cleaner code:

	ide_dev_drq_int(drive)

vs

	((drive)->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT)

We can of course have both things -- using macros for all checks
but still cache the result when it makes sense.

The idea of having macros for ->dev_flags and ->atapi_flags sounds
fine to me (+ it abstracts the actual ->dev_flags + ->atapi_flags
split allowing easier sanitizations / enhancements later).

Thanks,
Bart