linux-kernel - Re: [PATCH 4/9] usb: renesas: gadget: use generic map/unmap routines

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] [day] [month] [year] [list]

Date:	Tue, 20 Dec 2011 12:51:08 +0200
From:	Felipe Balbi <balbi@...com>
To:	Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
Cc:	Felipe Balbi <balbi@...com>,
	Linux USB Mailing List <linux-usb@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Thomas Dahlmann <dahlmann.thomas@...or.de>,
	"open list:DESIGNWARE USB3 D..." <linux-omap@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:AMD GEODE CS5536..." <linux-geode@...ts.infradead.org>
Subject: Re: [PATCH 4/9] usb: renesas: gadget: use generic map/unmap routines

Hi,

On Mon, Dec 19, 2011 at 05:46:56PM -0800, Kuninori Morimoto wrote:
> I tried this patch, but renesas_usbhs didn't work.
> It seems have some bugs.
> 
> 1. renesas_usbhs dma needs pkt->dma, but this patch didn't care it.
> 2. dma direction seems wrong
>    ("dir" needs 0/1, not DMA_xxx_DEVICE for usb_gadget_xx_request())
> 
> And, renesas_usbhs can not use scatter/gather type dma
> 
> #
> # this is not super important, but
> #
> # int usb_gadget_map_request(struct usb_gadget *gadget,
> #		struct usb_request *req, int direction)
> #
> # this "direction = 0/1" is a little bit un-understandable for me.
> # it is difficult to understand "which direction ?" from code.
> # if "direction = DMA_TO_DEVICE/DMA_FROM_DEVICE, it is understandable ;P

I see. The thing is that we want usb_gadget_map/unmap_request() to
handle that. In the long run, we might want to have a direction flag on
the public struct usb_request and remove this extra parameter, but I'm
still considering that :-)

> Can you please add below fix patch ?

thanks, will do as soon as you fix below :-)

> ------------------------------------------------
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index cb92a1d..c467067 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -173,15 +173,32 @@ static int usbhsg_dma_map_ctrl(struct usbhs_pkt *pkt, int map)
>  	struct usbhsg_uep *uep = usbhsg_pipe_to_uep(pipe);
>  	struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
>  	enum dma_data_direction dir;
> +	int ret;
>  
> -	dir = usbhs_pipe_is_dir_in(pipe) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +	dir = !usbhs_pipe_is_dir_in(pipe);
>  
>  	if (map) {
> -		return usb_gadget_map_request(&gpriv->gadget, req, dir);
> +		if (req->num_sgs) /* it can not use scatter/gather */
> +			return -EIO;

it would be better to through a big fat WARN() in this case. Gadget
drivers are supposed to check whether you support SGs (by looking into
gadget->sg_suported) before giving you a request with SGs.

> +		if (pkt->dma != DMA_ADDR_INVALID)
> +			return -EIO;

no no, we want to remove the whole DMA_ADDR_INVALID thing. What we're
doing now is that UDC is required to map the request buffer. So gadget
drivers must not do it. We shouldn't have this DMA_ADDR_INVALID macro
anymore :-)

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)