Re: [PATCH v1 2/4] exfat: use upcase_ptable and upcase_range_info to reduce memory footprint

[David Timber <dxdt@dev.snart.me>]

On 4/30/26 16:58, Yuezhang.Mo@sony.com wrote:
>> +static inline __u16 exfat_lookup_upcase_ptable(const struct exfat_upcase_ptable *ptbl,
>> +             const __u16 index)
>> +{
>> +     const size_t page_idx = index / EXFAT_UPTBL_PAGESIZE;
>> +     const size_t idx_in_page = index % EXFAT_UPTBL_PAGESIZE;
>> +
>> +     return ptbl->pages[page_idx] == NULL ? 0 : ptbl->pages[page_idx][idx_in_page];
>  
> How about this? Then we won't need to check it again in exfat_toupper().
>  
> return ptbl->pages[page_idx] == NULL ? index : ptbl->pages[page_idx][idx_in_page]; 

Zero(invalid character in exFAT spec) is used as identity within the
kernel. We'll have to be a bit careful. I'd have to change this logic in
exfat_load_upcase_table()

                ret = exfat_set_upcase_ptable(upcase_table, index, uni);
                if (ret) {
                    brelse(bh);
                    goto err;
                }

                def_upcase =
exfat_lookup_upcase_ptable(&default_upcase_table,
                                    index);
                is_default &= def_upcase == uni;

Some OOB data check will be ignored:

 - the character is "skipped": zero
 - the character is not skipped or specified as identity: non-zero

Subtle difference. If we change exfat_lookup_upcase_ptable() to return
identity on non-existent mapping, that information is lost, I think. But
again, the resulting contents would be the same regardless so I think
there isn't really a side effect.

>> +}
>> +
>> +void exfat_free_upcase_ptable(struct exfat_upcase_ptable *ptbl);
>> +int exfat_populate_upcase_ptable(struct exfat_upcase_ptable *ptbl,
>> +             const struct exfat_upcase_range_info *ri,
>> +             const size_t cnt);
>> +
>>  /* exfat/tables.c */
>>  /* Upcase table macro */
>> -#define EXFAT_NUM_UPCASE     (2918)
>> -#define EXFAT_UTBL_COUNT     (0x10000)
>> +#define EXFAT_DEF_UTBL_RI_COUNT      (112)
>> +#define EXFAT_DEF_UTBL_CHKSUM        (0xE619D30D)
>>  
>> -extern const unsigned short exfat_uni_def_upcase[EXFAT_NUM_UPCASE];
>> +extern const struct exfat_upcase_range_info exfat_def_utbl_ri[EXFAT_DEF_UTBL_RI_COUNT];
>  
> Is there a way to reduce the size of `exfat_def_utbl_ri` so that we
> can put it on the stack of `exfat_init_default_upcase_ptable()`?
>  
> If not, add `__initconst` for `exfat_def_utbl_ri`.
>  
Nah. It's as packed as it could get without breaking the alignment. We
could pack it down to 784 bytes, but that's still a lot of stack usage IMO.

>> +int exfat_init_default_upcase_ptable(void)
>  
> Add __init.
>  
>> +void exfat_free_default_upcase_ptable(void)
>  
> Add __exit.
Thanks.

>> @@ -896,6 +896,12 @@ static int __init init_exfat_fs(void)
>>  {
>>       int err;
>>  
>> +     err = exfat_init_default_upcase_ptable();
>> +     if (err) {
>> +             WARN_ON(err == -EINVAL);
>> +             return err;
>> +     }
>> +
>>       err = exfat_cache_init();
>>       if (err)
>>               return err;
>  
> Please add error handling to release default_upcase_ptable.
>  
>> +
>> +int exfat_populate_upcase_ptable(struct exfat_upcase_ptable *ptbl,
>> +             const struct exfat_upcase_range_info *ri,
>> +             const size_t cnt)
>                                                                                                                                                                                         
> This function is only called by exfat_init_default_upcase_ptable(),
> how about merge this function to it?
>  
default_upcase_ptable is already freed up before returning from
exfat_init_default_upcase_ptable() upon error. I made
exfat_populate_upcase_ptable() as a separate function to use it on my
test cases. The compiler inlines the function anyway(although
appropriate attributes are missing in the original patch, my bad) so
there's no ELF overhead either. I'd like to leave it as it is, if that's
okay.