v2:
- replaced kmalloc with kzalloc against memory initialization defects,
thus also removing a memset
- made error handling more consistent
v3:
- Split into smaller commits so that it's easier to review
v4:
- clearer commit messages
- added missing kfree
Felix Schlepper (3):
Staging: rtl8192e: Use struct_size
Staging: rtl8192e: Using kzalloc and delete memset
Staging: rtl8192e: Cleaning up error handling
drivers/staging/rtl8192e/rtllib_tx.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
--
2.36.1
By using kzalloc, we can delete a memset. The practical difference
is that using kzalloc() will zero out the txb->fragments[] array.
The original code worked fine, but zeroing everything seems nicer.
Signed-off-by: Felix Schlepper <[email protected]>
---
drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index f2ef32e943ae..1307cf55741a 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -205,11 +205,10 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
struct rtllib_txb *txb;
int i;
- txb = kmalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
+ txb = kzalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
if (!txb)
return NULL;
- memset(txb, 0, sizeof(struct rtllib_txb));
txb->nr_frags = nr_frags;
txb->frag_size = cpu_to_le16(txb_size);
--
2.36.1
Using struct_size is encouraged because it is safer
than using sizeof and calculations by hand.
Signed-off-by: Felix Schlepper <[email protected]>
---
drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index 37715afb0210..f2ef32e943ae 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -205,8 +205,7 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
struct rtllib_txb *txb;
int i;
- txb = kmalloc(sizeof(struct rtllib_txb) + (sizeof(u8 *) * nr_frags),
- gfp_mask);
+ txb = kmalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
if (!txb)
return NULL;
--
2.36.1
Moved error handling to one common block.
This removes double checking if all txb->fragments[]
were initialized.
The original code worked fine, but this is cleaner.
Signed-off-by: Felix Schlepper <[email protected]>
---
drivers/staging/rtl8192e/rtllib_tx.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index 1307cf55741a..42f81b23a144 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -214,19 +214,19 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
for (i = 0; i < nr_frags; i++) {
txb->fragments[i] = dev_alloc_skb(txb_size);
- if (unlikely(!txb->fragments[i])) {
- i--;
- break;
- }
+ if (unlikely(!txb->fragments[i]))
+ goto err_free;
memset(txb->fragments[i]->cb, 0, sizeof(txb->fragments[i]->cb));
}
- if (unlikely(i != nr_frags)) {
- while (i >= 0)
- dev_kfree_skb_any(txb->fragments[i--]);
- kfree(txb);
- return NULL;
- }
+
return txb;
+
+err_free:
+ while (--i >= 0)
+ dev_kfree_skb_any(txb->fragments[i]);
+ kfree(txb);
+
+ return NULL;
}
static int rtllib_classify(struct sk_buff *skb, u8 bIsAmsdu)
--
2.36.1
On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <[email protected]> wrote:
>
> Moved error handling to one common block.
> This removes double checking if all txb->fragments[]
> were initialized.
> The original code worked fine, but this is cleaner.
...
> +err_free:
> + while (--i >= 0)
while (i--)
will suffice.
> + dev_kfree_skb_any(txb->fragments[i]);
> + kfree(txb);
> +
> + return NULL;
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 23, 2022 at 11:20:49PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <[email protected]> wrote:
> >
> > Moved error handling to one common block.
> > This removes double checking if all txb->fragments[]
> > were initialized.
> > The original code worked fine, but this is cleaner.
>
> ...
>
> > +err_free:
> > + while (--i >= 0)
>
> while (i--)
>
> will suffice.
>
Either one is fine. You prefer this format. I prefer that other
format. I told Felix he could use either format without expressing any
bias and he chose my format. That means he loves me more.
regards,
dan carpenter
On 24.06.2022 08:21, Dan Carpenter wrote:
> On Thu, Jun 23, 2022 at 11:20:49PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <[email protected]> wrote:
> > >
> > > Moved error handling to one common block.
> > > This removes double checking if all txb->fragments[]
> > > were initialized.
> > > The original code worked fine, but this is cleaner.
> >
> > ...
> >
> > > +err_free:
> > > + while (--i >= 0)
> >
> > while (i--)
> >
> > will suffice.
> >
>
> Either one is fine. You prefer this format. I prefer that other
> format. I told Felix he could use either format without expressing any
> bias and he chose my format. That means he loves me more.
>
> regards,
> dan carpenter
Andy's advice is of course much appreciated but I prefer Dan's style <3.
On another note, the thread is quite messy now, since all my previous
failed attempts to send are now threaded to this one.
How would I go about solving this? Sending another v5 version, the v4
with [RESEND PATCH v4...] or does this even need any action on my part?
Cheers
Felix Schlepper
On Fri, Jun 24, 2022 at 10:41:42AM +0200, Felix Schlepper wrote:
> On 24.06.2022 08:21, Dan Carpenter wrote:
> > On Thu, Jun 23, 2022 at 11:20:49PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <[email protected]> wrote:
> > > >
> > > > Moved error handling to one common block.
> > > > This removes double checking if all txb->fragments[]
> > > > were initialized.
> > > > The original code worked fine, but this is cleaner.
> > >
> > > ...
> > >
> > > > +err_free:
> > > > + while (--i >= 0)
> > >
> > > while (i--)
> > >
> > > will suffice.
> > >
> >
> > Either one is fine. You prefer this format. I prefer that other
> > format. I told Felix he could use either format without expressing any
> > bias and he chose my format. That means he loves me more.
> >
> > regards,
> > dan carpenter
> Andy's advice is of course much appreciated but I prefer Dan's style <3.
>
> On another note, the thread is quite messy now, since all my previous
> failed attempts to send are now threaded to this one.
> How would I go about solving this? Sending another v5 version, the v4
> with [RESEND PATCH v4...] or does this even need any action on my part?
Please resend a v5, as I now have 3 different copies of a v4 series in
my inbox, which makes no sense at all.
Remember, make it obvious as to what to do for those of us who have to
handle 1000+ emails a day...
thanks,
greg k-h