Re: [PATCH 1/3] ata: make packed transfer mode masks *unsigned int*

Damien Le Moal
2022-05-20T03:37:15Z

On 5/20/22 05:19, Sergey Shtylyov wrote:
> Hello!
>
> On 5/16/22 2:17 PM, Damien Le Moal wrote:
>
>>> The packed transfer mode masks are declared as *unsigned long* (which is
>>> a 64-bit type on 64-bit architectures), however the actual mask occupies
>>> only 20 bits (7 PIO modes, 5 MWDMA modes, and 8 UDMA modes), so we can
>>> safely use 32-bit *unsigned int* variables instead. Convert all libata
>>> functions taking as a parameter or returning a packed transfer mode mask.
>>> This saves 470 bytes of object code in libata-core.o alone...
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> [...]
>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>> index 732de9014626..1429b7012ae8 100644
>>> --- a/include/linux/libata.h
>>> +++ b/include/linux/libata.h
> [...]
>>> @@ -1103,16 +1100,18 @@ extern void ata_msleep(struct ata_port *ap, unsigned int msecs);
>>> extern u32 ata_wait_register(struct ata_port *ap, void __iomem *reg, u32 mask,
>>> u32 val, unsigned long interval, unsigned long timeout);
>>> extern int atapi_cmd_type(u8 opcode);
>>> -extern unsigned long ata_pack_xfermask(unsigned long pio_mask,
>>> - unsigned long mwdma_mask, unsigned long udma_mask);
>>> -extern void ata_unpack_xfermask(unsigned long xfer_mask,
>>> - unsigned long *pio_mask, unsigned long *mwdma_mask,
>>> - unsigned long *udma_mask);
>>> -extern u8 ata_xfer_mask2mode(unsigned long xfer_mask);
>>> -extern unsigned long ata_xfer_mode2mask(u8 xfer_mode);
>>> +extern unsigned int ata_pack_xfermask(unsigned long pio_mask,
>>> + unsigned long mwdma_mask,
>>> + unsigned long udma_mask);
>>> +extern void ata_unpack_xfermask(unsigned int xfer_mask,
>>> + unsigned long *pio_mask,
>>> + unsigned long *mwdma_mask,
>>> + unsigned long *udma_mask);
>>
>> Why not change all of these to unsigned int too ?
>
> Done in the 2nd patch.
>
>> They are defined as "1LU << shift" but everything actually fits within 32 bits
>
> No, they are #define'd as *int* masks in ata.h, not as *unsigned long* in libata.h...

I meant the mask macro values used to set these fields are all defined as
unsigned long with "1LU << shift" defines. See enum ata_xfer_mask in libata.h.

>
> [...]
>
> MBR, Sergey

--
Damien Le Moal
Western Digital Research