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

Sergey Shtylyov
2022-06-10T20:19:26Z

On 5/20/22 6:37 AM, 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.

What does it have to do with {pio|mwdma|udma}_mask? These *enum*s values are
used solely for the packed mask...

MBR, Sergey