| Message ID | 20220508204139.4961-1-s.shtylyov@omp.ru |
|---|---|
| Headers | show |
| Series | Make PATA transfer mode masks always being 32-bit | expand |
On 5/8/22 11:41 PM, Sergey Shtylyov 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... Forgot to mention the patchset was against the 'for-next' branch of Damien's 'libata.git' repo. In fact, I did even forgot to use a working branch while preparing the patches to posting -- did that atop of the real local copy of the 'for-next' branch. :-/ [...] MBR, Sergey
On 5/9/22 05:41, Sergey Shtylyov 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 ?
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. MBR, Sergey
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
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... :-/ > 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.... MBR, Sergey
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
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(-)