linux-kernel - Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130730140059.GE27962@kroah.com>
Date:	Tue, 30 Jul 2013 07:00:59 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Hayes Wang <hayeswang@...ltek.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, nic_swsd@...ltek.com
Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to
 DMA-able

On Tue, Jul 30, 2013 at 04:28:54PM +0800, Hayes Wang wrote:
> Allocate the transfer buffer in probe(), and use the buffer for
> usb control transfer.
> 
> Signed-off-by: Hayes Wang <hayeswang@...ltek.com>
> ---
>  drivers/net/usb/r815x.c | 117 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 90 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
> index 8523922..89ad63f 100644
> --- a/drivers/net/usb/r815x.c
> +++ b/drivers/net/usb/r815x.c
> @@ -21,37 +21,52 @@
>  #define R815x_PHY_ID		32
>  #define REALTEK_VENDOR_ID	0x0bda
>  
> +struct r815x {
> +	struct mutex transfer_mutex;
> +	__le32 *transfer_buf;
> +};

Really?  Why not just allocate it the times you need to send the data?
Is it really all that often that you need to do this?

> -static int pla_read_word(struct usb_device *udev, u16 index)
> +static int pla_read_word(struct usbnet *dev, u16 index)
>  {
> -	int data, ret;
> +	struct r815x *tp = dev->driver_priv;
> +	struct usb_device *udev = dev->udev;
> +	int ret;
>  	u8 shift = index & 2;
> -	__le32 ocp_data;
> +	__le32 *tmp;
> +
> +	mutex_lock(&tp->transfer_mutex);
> +	tmp = tp->transfer_buf;
>  
>  	index &= ~3;
>  
>  	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>  			      RTL815x_REQ_GET_REGS, RTL815x_REQT_READ,
> -			      index, MCU_TYPE_PLA, &ocp_data, sizeof(ocp_data),
> -			      500);
> +			      index, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);

This call is so slow, you can afford to make a call to kmalloc for the
data, as it sure just did for other structures it needed :)

So just do a kmalloc call for the data before this function, same for
your other patches in this series, no need for a lock for your "transfer
data" buffer, and a global one at all.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/