Re: [PATCH 2/2] tcm_usb_gadget: Fix nexus leak + enabled attribute failure

[Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>]

Hi Nicholas,

W dniu 27.09.2015 o 05:16, Nicholas A. Bellinger pisze:
> From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
>
> This patch adds the missing tcm_usbg_drop_nexus() to properly
> release tcm_usbg_nexus memory during typical ->fabric_drop_tpg()
> callback shutdown.
>
> Also, fix up tcm_usbg_tpg_store_enable() return value to propigate
> usbg_attach() failure up to user-space if no HDC is found.

Are these two changes related? If not, maybe a separate patch?

> Reported-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Cc: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> ---
>   drivers/usb/gadget/legacy/tcm_usb_gadget.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/legacy/tcm_usb_gadget.c b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
> index b764e91..54f036b 100644
> --- a/drivers/usb/gadget/legacy/tcm_usb_gadget.c
> +++ b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
> @@ -1425,11 +1425,14 @@ static struct se_portal_group *usbg_make_tpg(
>   	return &tpg->se_tpg;
>   }
>
> +static int tcm_usbg_drop_nexus(struct usbg_tpg *);
> +
>   static void usbg_drop_tpg(struct se_portal_group *se_tpg)
>   {
>   	struct usbg_tpg *tpg = container_of(se_tpg,
>   				struct usbg_tpg, se_tpg);
>
> +	tcm_usbg_drop_nexus(tpg);
>   	core_tpg_deregister(se_tpg);
>   	destroy_workqueue(tpg->workqueue);
>   	kfree(tpg);
> @@ -1507,10 +1510,14 @@ static ssize_t tcm_usbg_tpg_store_enable(
>   	if (op > 1)
>   		return -EINVAL;
>
> -	if (op && tpg->gadget_connect)
> +	if (op && tpg->gadget_connect) {
> +		ret = -EINVAL;
>   		goto out;

And @out there is only return ret. Why not return -EINVAL here?


> -	if (!op && !tpg->gadget_connect)
> +	}
> +	if (!op && !tpg->gadget_connect) {
> +		ret = -EINVAL;
>   		goto out;

ditto


Then in fact the function body could be shorter:

....
	ssize_t ret = 0;

	ret = kstrtoul(page, 0, &op);
	if (ret < 0 || op > 1)
		return -EINVAL;

	if (op && tpg->gadget_connect || !op && !tpg->gadget_connect)
		return -EINVAL;

	if (op)
		ret = usbg_attach(tpg);
	else
		usbg_detach(tpg);

	if (ret)
		return ret;

	tpg->gadget_connect = op;

	return ret;
}

And even simpler if strtobool() were used instead of kstrtoul().

If, on the other hand, you want to have a patch which is as simple
as possible you can get away with:

....
	ssize_t ret = -EINVAL;


and
....

	tpg->gagget_connect = op;
	ret = count;

	return ret;

AP


--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html