From: Dan Carpenter <dan.carpenter@oracle.com>
To: Roi Martin <jroi.martin@gmail.com>
Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org,
valdis.kletnieks@vt.edu, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 6/6] staging: exfat: replace kmalloc with kmalloc_array
Date: Fri, 1 Nov 2019 12:34:47 +0300 [thread overview]
Message-ID: <20191101093447.GG18421@kadam> (raw)
In-Reply-To: <20191031160356.GB6924@miniwopr.localdomain>
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
prev parent reply other threads:[~2019-11-01 9:35 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-31 12:31 [PATCH v2 6/6] staging: exfat: replace kmalloc with kmalloc_array Roi Martin
2019-10-31 13:25 ` Dan Carpenter
2019-10-31 16:03 ` Roi Martin
2019-11-01 9:34 ` Dan Carpenter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191101093447.GG18421@kadam \
--to=dan.carpenter@oracle.com \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=jroi.martin@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=valdis.kletnieks@vt.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).