The return type of usbhsp_setup_pipecfg() was u16 but it was returning
a negative value (-EINVAL). Instead lets return a pointer to u16 which
will hold the value to be returned or in case of error, return the
error code in ERR_PTR.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/usb/renesas_usbhs/pipe.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/renesas_usbhs/pipe.c b/drivers/usb/renesas_usbhs/pipe.c
index 78e9dba..00d595c 100644
--- a/drivers/usb/renesas_usbhs/pipe.c
+++ b/drivers/usb/renesas_usbhs/pipe.c
@@ -391,9 +391,9 @@ void usbhs_pipe_set_trans_count_if_bulk(struct usbhs_pipe *pipe, int len)
/*
* pipe setup
*/
-static u16 usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
- int is_host,
- int dir_in)
+static u16 *usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
+ int is_host,
+ int dir_in)
{
u16 type = 0;
u16 bfre = 0;
@@ -407,9 +407,13 @@ static u16 usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
[USB_ENDPOINT_XFER_INT] = TYPE_INT,
[USB_ENDPOINT_XFER_ISOC] = TYPE_ISO,
};
+ u16 *result;
if (usbhs_pipe_is_dcp(pipe))
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
+ result = kmalloc(sizeof(u16), GFP_KERNEL);
+ if (!result)
+ return ERR_PTR(-ENOMEM);
/*
* PIPECFG
@@ -451,14 +455,14 @@ static u16 usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
/* EPNUM */
epnum = 0; /* see usbhs_pipe_config_update() */
-
- return type |
- bfre |
- dblb |
- cntmd |
- dir |
- shtnak |
- epnum;
+ *result = type |
+ bfre |
+ dblb |
+ cntmd |
+ dir |
+ shtnak |
+ epnum;
+ return result;
}
static u16 usbhsp_setup_pipebuff(struct usbhs_pipe *pipe)
@@ -683,6 +687,7 @@ struct usbhs_pipe *usbhs_pipe_malloc(struct usbhs_priv *priv,
int is_host = usbhs_mod_is_host(priv);
int ret;
u16 pipecfg, pipebuf;
+ u16 *result;
pipe = usbhsp_get_pipe(priv, endpoint_type);
if (!pipe) {
@@ -702,7 +707,14 @@ struct usbhs_pipe *usbhs_pipe_malloc(struct usbhs_priv *priv,
return NULL;
}
- pipecfg = usbhsp_setup_pipecfg(pipe, is_host, dir_in);
+ result = usbhsp_setup_pipecfg(pipe, is_host, dir_in);
+ if (IS_ERR(result)) {
+ dev_err(dev, "can't setup pipe\n");
+ return NULL;
+ }
+ pipecfg = *result;
+ kfree(result);
+
pipebuf = usbhsp_setup_pipebuff(pipe);
usbhsp_pipe_select(pipe);
--
1.9.1
Hi,
> From: Sudip Mukherjee
> Sent: Saturday, April 09, 2016 12:05 AM
>
> The return type of usbhsp_setup_pipecfg() was u16 but it was returning
> a negative value (-EINVAL). Instead lets return a pointer to u16 which
> will hold the value to be returned or in case of error, return the
> error code in ERR_PTR.
Thank you for the patch!
I also think this usbhsp_setup_pipecfg() should return error code using correct variable type.
However, I would like to avoid to use ERR_PTR and kmalloc() somehow because
I feel this patch is complex a little.
How about the usbhsp_setup_pipecfg() prototype is changed like the following?
static int usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
int is_host, int dir_in, u16 *pipecfg);
Best regards,
Yoshihiro Shimoda
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/usb/renesas_usbhs/pipe.c | 38 +++++++++++++++++++++++++-------------
> 1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/renesas_usbhs/pipe.c b/drivers/usb/renesas_usbhs/pipe.c
> index 78e9dba..00d595c 100644
> --- a/drivers/usb/renesas_usbhs/pipe.c
> +++ b/drivers/usb/renesas_usbhs/pipe.c
> @@ -391,9 +391,9 @@ void usbhs_pipe_set_trans_count_if_bulk(struct usbhs_pipe *pipe, int len)
> /*
> * pipe setup
> */
> -static u16 usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
> - int is_host,
> - int dir_in)
> +static u16 *usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
> + int is_host,
> + int dir_in)
> {
> u16 type = 0;
> u16 bfre = 0;
> @@ -407,9 +407,13 @@ static u16 usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
> [USB_ENDPOINT_XFER_INT] = TYPE_INT,
> [USB_ENDPOINT_XFER_ISOC] = TYPE_ISO,
> };
> + u16 *result;
>
> if (usbhs_pipe_is_dcp(pipe))
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
> + result = kmalloc(sizeof(u16), GFP_KERNEL);
> + if (!result)
> + return ERR_PTR(-ENOMEM);
>
> /*
> * PIPECFG
> @@ -451,14 +455,14 @@ static u16 usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
>
> /* EPNUM */
> epnum = 0; /* see usbhs_pipe_config_update() */
> -
> - return type |
> - bfre |
> - dblb |
> - cntmd |
> - dir |
> - shtnak |
> - epnum;
> + *result = type |
> + bfre |
> + dblb |
> + cntmd |
> + dir |
> + shtnak |
> + epnum;
> + return result;
> }
>
> static u16 usbhsp_setup_pipebuff(struct usbhs_pipe *pipe)
> @@ -683,6 +687,7 @@ struct usbhs_pipe *usbhs_pipe_malloc(struct usbhs_priv *priv,
> int is_host = usbhs_mod_is_host(priv);
> int ret;
> u16 pipecfg, pipebuf;
> + u16 *result;
>
> pipe = usbhsp_get_pipe(priv, endpoint_type);
> if (!pipe) {
> @@ -702,7 +707,14 @@ struct usbhs_pipe *usbhs_pipe_malloc(struct usbhs_priv *priv,
> return NULL;
> }
>
> - pipecfg = usbhsp_setup_pipecfg(pipe, is_host, dir_in);
> + result = usbhsp_setup_pipecfg(pipe, is_host, dir_in);
> + if (IS_ERR(result)) {
> + dev_err(dev, "can't setup pipe\n");
> + return NULL;
> + }
> + pipecfg = *result;
> + kfree(result);
> +
> pipebuf = usbhsp_setup_pipebuff(pipe);
>
> usbhsp_pipe_select(pipe);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
Yoshihiro Shimoda <[email protected]> writes:
>> From: Sudip Mukherjee
>> Sent: Saturday, April 09, 2016 12:05 AM
>>
>> The return type of usbhsp_setup_pipecfg() was u16 but it was returning
>> a negative value (-EINVAL). Instead lets return a pointer to u16 which
>> will hold the value to be returned or in case of error, return the
>> error code in ERR_PTR.
>
> Thank you for the patch!
> I also think this usbhsp_setup_pipecfg() should return error code using correct variable type.
>
> However, I would like to avoid to use ERR_PTR and kmalloc() somehow because
> I feel this patch is complex a little.
> How about the usbhsp_setup_pipecfg() prototype is changed like the following?
>
> static int usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
> int is_host, int dir_in, u16 *pipecfg);
IMO, this makes much more sense.
--
balbi
On Thursday 14 April 2016 04:25 PM, Felipe Balbi wrote:
>
> Hi,
>
> Yoshihiro Shimoda <[email protected]> writes:
>>> From: Sudip Mukherjee
>>> Sent: Saturday, April 09, 2016 12:05 AM
>>>
>>> The return type of usbhsp_setup_pipecfg() was u16 but it was returning
>>> a negative value (-EINVAL). Instead lets return a pointer to u16 which
>>> will hold the value to be returned or in case of error, return the
>>> error code in ERR_PTR.
>>
>> Thank you for the patch!
>> I also think this usbhsp_setup_pipecfg() should return error code using correct variable type.
>>
>> However, I would like to avoid to use ERR_PTR and kmalloc() somehow because
>> I feel this patch is complex a little.
>> How about the usbhsp_setup_pipecfg() prototype is changed like the following?
>>
>> static int usbhsp_setup_pipecfg(struct usbhs_pipe *pipe,
>> int is_host, int dir_in, u16 *pipecfg);
>
> IMO, this makes much more sense.
Infact, I thought about both these ways while making the patch but
somehow I thought this one is a better but ofcourse the other way is
much simpler.
I will post v2.
regards
sudip