Re: [PATCH v2 6/6] staging: exfat: replace kmalloc with kmalloc_array

[Dan Carpenter]

On Thu, Oct 31, 2019 at 05:03:56PM +0100, Roi Martin wrote:
> > > diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c
> > > index f71235c6a338..f4f82aecc05d 100644
> > > --- a/drivers/staging/exfat/exfat_core.c
> > > +++ b/drivers/staging/exfat/exfat_core.c
> > > @@ -713,8 +713,8 @@ static s32 __load_upcase_table(struct super_block *sb, sector_t sector,
> > >  
> > >  	u32 checksum = 0;
> > >  
> > > -	upcase_table = p_fs->vol_utbl = kmalloc(UTBL_COL_COUNT * sizeof(u16 *),
> > > -						GFP_KERNEL);
> > > +	upcase_table = kmalloc_array(UTBL_COL_COUNT, sizeof(u16 *), GFP_KERNEL);
> > > +	p_fs->vol_utbl = upcase_table;
> > 
> > This patch is fine, but one idea for future patches is that you could
> > remove the "upcase_table" variable and use "p_fs->vol_utbl" everywhere
> > instead.
> 
> Thanks for the suggestion.
> 
> This is my first contribution and I tried to introduce the minimum
> number of changes necessary to fix the issues reported by checkpatch.pl.
> Also, I'm still immersed in getting familiar with the contribution
> process and the code.
> 
> Do you think it makes sense to include this change in a future patch
> series along with other refactoring? Or, should I modify this patch?

No don't modify the patch.  The patch is fine.

> 
> By the way, upcase_table is sometimes accessed in quite complex ways.
> For instance:
> 
> 	upcase_table[col_index][get_row_index(index)] = uni;
> 
> Where having an intermediate variable instead of using the struct field
> directly seems to improve readability a bit. Otherwise:
> 
> 	p_fs->vol_utbl[col_index][get_row_index(index)] = uni;

This line isn't very complex.  It's fine.


> 
> I assume, in cases like this, from a coding style perspective, the
> following approach is preferred:
> 
> 	row_index = get_row_index(index);
> 	p_fs->vol_utbl[col_index][row_index] = uni;

But this is better, yes.

regards,
dan carpenter