linux-kernel - Re: [PATCH 5/6] ide: remove ide_execute_pkt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Feb 2009 14:37:40 +0100
From:	Borislav Petkov <petkovbb@...glemail.com>
To:	Sergei Shtylyov <sshtylyov@...mvista.com>
Cc:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()

Hi,

On Wed, Feb 11, 2009 at 2:22 PM, Sergei Shtylyov
<sshtylyov@...mvista.com> wrote:
> Hello.
>
> Borislav Petkov wrote:
>
>>> From: Bartlomiej Zolnierkiewicz <bzolnier@...il.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@...il.com>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...il.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 Conﬁguration Word.

>> 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?

>> 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.

>> If it's ok with you, I'll whip up patch(es) later.
>>
>
>>>        if (dev_is_idecd(drive)) {
>>>                tf_flags = IDE_TFLAG_OUT_NSECT | IDE_TFLAG_OUT_LBAL;
>>> @@ -675,17 +677,14 @@ ide_startstop_t ide_issue_pc(ide_drive_t
>>>
>
>  Don't leave uncommented fragment behind please.
>
> MBR, Sergei
>
>

[..]


-- 
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/