On 6/11/22 06:20, Sergey Shtylyov wrote:
> On 6/8/22 9:44 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.
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.
But given that patch 1 and 2 both touch the same functions, one patch
would be better.
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
