2014-10-14 16:40:43

by Fabian Frédérick

[permalink] [raw]
Subject: [PATCH 1/1 linux-next] zd1201: replace kmalloc/memset by kzalloc

Signed-off-by: Fabian Frederick <[email protected]>
---
drivers/net/wireless/zd1201.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/zd1201.c b/drivers/net/wireless/zd1201.c
index 6f5c793..4630b26 100644
--- a/drivers/net/wireless/zd1201.c
+++ b/drivers/net/wireless/zd1201.c
@@ -525,7 +525,7 @@ static int zd1201_setconfig(struct zd1201 *zd, int rid, void *buf, int len, int
zd->rxdatas = 0;
zd->rxlen = 0;
for (seq=0; len > 0; seq++) {
- request = kmalloc(16, gfp_mask);
+ request = kzalloc(16, gfp_mask);
if (!request)
return -ENOMEM;
urb = usb_alloc_urb(0, gfp_mask);
@@ -533,7 +533,6 @@ static int zd1201_setconfig(struct zd1201 *zd, int rid, void *buf, int len, int
kfree(request);
return -ENOMEM;
}
- memset(request, 0, 16);
reqlen = len>12 ? 12 : len;
request[0] = ZD1201_USB_RESREQ;
request[1] = seq;
--
1.9.3


2014-10-14 17:16:01

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] zd1201: replace kmalloc/memset by kzalloc

On Tue, 2014-10-14 at 18:40 +0200, Fabian Frederick wrote:
> Signed-off-by: Fabian Frederick <[email protected]>

It might be clearer to use a structure for this 16 byte thing.

There's a comment bit in the code:

/* cmdreq message:
u32 type
u16 cmd
u16 parm0
u16 parm1
u16 parm2
u8 pad[4]

total: 4 + 2 + 2 + 2 + 2 + 4 = 16
*/

It seems thought that this first u32 is actually
a series of u8s.

Maybe:

struct zd1201_cmdreq {
u8 type;
u8 seq;
u8 a;
u8 b;
__le16 cmd;
__le16 parm0;
__le16 parm1;
__le16 parm2;
u8 pad[4];
};

And does this really need to be alloc'd?

maybe

struct zd1202_cmdreq request = {};

etc...

2014-10-14 18:09:08

by Bjørn Mork

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] zd1201: replace kmalloc/memset by kzalloc

Joe Perches <[email protected]> writes:

> And does this really need to be alloc'd?

Yes, it does. It is used as a transfer_buffer in usb_fill_bulk_urb() and
must be "suitable for DMA". From include/linux/usb.h:

/**
* struct urb - USB Request Block
..
* @transfer_buffer: This identifies the buffer to (or from) which the I/O
* request will be performed unless URB_NO_TRANSFER_DMA_MAP is set
* (however, do not leave garbage in transfer_buffer even then).
* This buffer must be suitable for DMA; allocate it with
* kmalloc() or equivalent. For transfers to "in" endpoints, contents
* of this buffer will be modified. This buffer is used for the data
* stage of control transfers.



Bjørn

2014-10-16 08:08:56

by Fabian Frédérick

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] zd1201: replace kmalloc/memset by kzalloc



> On 14 October 2014 at 20:08 Bjørn Mork <[email protected]> wrote:
>
>
> Joe Perches <[email protected]> writes:
>
> > And does this really need to be alloc'd?
>
> Yes, it does. It is used as a transfer_buffer in usb_fill_bulk_urb() and
> must be "suitable for DMA".  From include/linux/usb.h:
>
> /**
>  * struct urb - USB Request Block
> ..
>  * @transfer_buffer:  This identifies the buffer to (or from) which the I/O
>  *      request will be performed unless URB_NO_TRANSFER_DMA_MAP is set
>  *      (however, do not leave garbage in transfer_buffer even then).
>  *      This buffer must be suitable for DMA; allocate it with
>  *      kmalloc() or equivalent.  For transfers to "in" endpoints, contents
>  *      of this buffer will be modified.  This buffer is used for the data
>  *      stage of control transfers.
>
>

Adding a structure would be a nice design update indeed but this would require
some testing...Separate patch maybe ?
 
Fabian

>
> Bjørn

2014-10-16 08:50:48

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] zd1201: replace kmalloc/memset by kzalloc

On Thu, 2014-10-16 at 10:08 +0200, Fabian Frederick wrote:
> Adding a structure would be a nice design update indeed but this would require
> some testing...Separate patch maybe ?

Of course that'd be fine.