On 6/14/22 04:20, Sergey Shtylyov wrote:
> Hello!
>
> On 6/13/22 2:24 AM, Damien Le Moal wrote:
>
>>>>> The PATA transfer mode masks (direct and packed) in libata are sometimes
>>>>> declared as *unsigned int* and sometimes as *unsigned long* (which is a
>>>>> 64-bit type on 64-bit architectures), while the packed mask really only
>>>>> uses 20 bits and the PIO/MWDMA/UDMA masks use up to 8 bits. Switching to
>>>>> the uniform 32-bit masks saves siginificant amount of the object code...
>>>>>
>>>>> Sergey Shtylyov (3):
>>>>> ata: make packed transfer mode masks *unsigned int*
>>>>> ata: make ata_device::{pio|mwdma|udma}_mask *unsigned int*
>>>>> ata: make ata_port_info::{pio|mwdma|udma}_mask *unsigned int*
>>>>>
>>>>> drivers/ata/libata-acpi.c | 8 +++---
>>>>> drivers/ata/libata-core.c | 38 +++++++++++++-------------
>>>>> drivers/ata/pata_acpi.c | 2 +-
>>>>> drivers/ata/pata_ali.c | 2 +-
>>>>> drivers/ata/pata_amd.c | 14 +++++-----
>>>>> drivers/ata/pata_hpt366.c | 2 +-
>>>>> drivers/ata/pata_hpt37x.c | 6 ++---
>>>>> drivers/ata/pata_hpt3x2n.c | 2 +-
>>>>> drivers/ata/pata_pdc2027x.c | 4 +--
>>>>> drivers/ata/pata_serverworks.c | 4 +--
>>>>> drivers/ata/pata_sis.c | 2 +-
>>>>> drivers/ata/pata_via.c | 2 +-
>>>>> include/linux/libata.h | 49 +++++++++++++++++-----------------
>>>>> 13 files changed, 67 insertions(+), 68 deletions(-)
>>>>>
>>>>
>>>> Are you going to resend this as a single patch ?
>>>
>>> No, I'd like to avoid that... Please merge as is.
>>
>> Nope. I still have concerns about this patch structure. And reviewing
>> again, I think some changes are still missing.
>> E.g., patch 3 changes struct ata_port_info masks to unsigned int. This is
>> used in ata_host_alloc_pinfo() to set the port masks, but I do not see
>> where these are changed to unsigned int too. Which patch does that ? These
>> should be in the same patch.
>
> Sigh... they always were *unsigned int*! And I explicitly wrote about that
> in the commit msg of that patch... :-/
Nobody is perfect. I miss/forget stuff from time to time. All you need to
do is to remind me. So no need for the "sigh".
>
>> I am OK with one patch for the packed mask, and one patch for the
>> {pio|mwdma|udma}_mask fields. Patch 3 is weird and should at least be
>> squashed into patch 2.
>
> Why? What does *struct* ata_device have to do with *struct* ata_port_info?
>
>>
>> But given that patch 1 and 2 both touch the same functions, one patch
>> would be better.
>
> I specifically aimed at minimizing the patches....
I do not care about the number of patches it takes to fix/improve
something. I care about what the patches do. And for this case, I prefer
seeing a single patch cleaning up the mess of unsigned long/unsigned int
for all these transfer speed masks so that everything becomes unsigned int
in one go. With multiple patches we still have a messy situation of mixed
unsigned long/unsigned int between patches.
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
