linux-kernel - Re: [PATCH v2] ata: add CONFIG_SATA

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 for Android: free password hash cracker in your pocket

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

Message-ID: <AANLkTin60En3+5saCeVuGht8w0k1b074WqKLdmo1WE5T@mail.gmail.com>
Date:	Mon, 14 Feb 2011 09:36:49 +0100
From:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
To:	Sergei Shtylyov <sshtylyov@...sta.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
	Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [PATCH v2] ata: add CONFIG_SATA_HOST config option

Hi,

On Sun, Feb 13, 2011 at 3:39 PM, Sergei Shtylyov <sshtylyov@...sta.com> wrote:
> Hello.
>
> On 11-02-2011 16:33, Bartlomiej Zolnierkiewicz wrote:
>
>> Add CONFIG_SATA_HOST config option (for selecting SATA Host
>> support) to make setup easier on PATA-only systems.
>
>> Additionally move SATA-specific code to libata-sata.c which
>> allows us to save ~11.5k of the output code size (x86-64) on
>> PATA-only systems for CONFIG_SATA_HOST=n:
>
>> CONFIG_SATA_HOST=y:
>>    text    data     bss     dec     hex filename
>>   44283    6576      57   50916    c6e4 drivers/ata/libata-core.o
>>   29054      16       2   29072    7190 drivers/ata/libata-eh.o
>>   20085       0      19   20104    4e88 drivers/ata/libata-sff.o
>>    8699       0       0    8699    21fb drivers/ata/libata-sata.o
>
>> CONFIG_SATA_HOST=n:
>>    text    data     bss     dec     hex filename
>>   43754    6576      57   50387    c4d3 drivers/ata/libata-core.o
>>   26775      16       2   26793    68a9 drivers/ata/libata-eh.o
>>   20144       0      19   20163    4ec3 drivers/ata/libata-sff.o
>
>> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@...il.com>
>
> [...]
>
>> Index: b/drivers/ata/libata-core.c
>> ===================================================================
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>
> [...]
>>
>> @@ -573,34 +494,26 @@ void ata_tf_to_fis(const struct ata_task
>>        fis[19] = 0;
>>  }
>
>   Hm, what about ata_tf_to_fis()? It shouldn't be needed by non-SATA stuff,
> moreover it'should be only needed by non-SFF controllers...

in theory yes but it is actually used by libata-scsi.c

>> +#ifndef CONFIG_SATA_HOST
>> +int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>> +                     bool spm_wakeup)
>>  {
>> -       tf->command     = fis[2];       /* status */
>> -       tf->feature     = fis[3];       /* error */
>> -
>> -       tf->lbal        = fis[4];
>> -       tf->lbam        = fis[5];
>> -       tf->lbah        = fis[6];
>> -       tf->device      = fis[7];
>> -
>> -       tf->hob_lbal    = fis[8];
>> -       tf->hob_lbam    = fis[9];
>> -       tf->hob_lbah    = fis[10];
>> -
>> -       tf->nsect       = fis[12];
>> -       tf->hob_nsect   = fis[13];
>> +       return -EOPNOTSUPP;
>> +}
>> +EXPORT_SYMBOL_GPL(sata_link_scr_lpm);
>
>   Shouldn't there be empty line here?

ata_piix needs it

>> +int sata_std_hardreset(struct ata_link *link, unsigned int *class,
>> +                      unsigned long deadline)
>> +{
>> +       return -EOPNOTSUPP;
>> +}
>> +EXPORT_SYMBOL_GPL(sata_std_hardreset);
>
>   ... and here?

ditto, and it is quite useful to have possibility of building ata_piix
w/ CONFIG_SATA_HOST=n

>> Index: b/drivers/ata/libata-sata.c
>> ===================================================================
>> --- /dev/null
>> +++ b/drivers/ata/libata-sata.c
>> @@ -0,0 +1,1242 @@
>
> [...]
>>
>> +int sata_set_spd(struct ata_link *link)
>> +{
>> +       u32 scontrol;
>> +       int rc;
>> +
>> +       if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
>
>   I guess checkpatch.pl protests here...

it does ;)

[...]

>> +               goto out;
>> +
>> +       scontrol = (scontrol&  0x0f0) | 0x301;
>> +
>> +       if ((rc = sata_scr_write_flush(link, SCR_CONTROL, scontrol)))
>
>   And here...
>   I understand that you're only moving the code. Perhaps it's worth fixing
> checkpatch.pl's complaints beforehand -- I was going to look at it...

Because code is moved I preferred to leave CodingStyle changes alone
but I also think that it would be useful to fix them one day (though I
deferred the work for later due to pragmatic reasons)..

Thanks,
Bartlomiej
--
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/