Received-SPF: pass (google.com: domain of linux-kernel+bounces-169031-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sun, 5 May 2024 23:31:44 +0800
From: Hongren Zheng <i@zenithal.me>
To: Simon Holesch <simon@holesch.de>
Cc: Valentina Manea <valentina.manea.m@gmail.com>,
	Shuah Khan <shuah@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Shuah Khan <skhan@linuxfoundation.org>, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] usbip: Don't submit special requests twice
Message-ID: <ZjemYB6CpAx4Kx5f@Sun>
References: <20231217194624.102385-1-simon@holesch.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231217194624.102385-1-simon@holesch.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?GTfmgmq3eK16dIv12BxkTSv7lcWz57ath+6h2M7tsIFoCe6E333fa1Ku6S7h?=
 =?us-ascii?Q?kDp+DgWHn32h4AR3/+t5qA8EuW2h8wvRffmdnSUZuTA+hkNKbJsqZQhQ/aec?=
 =?us-ascii?Q?zZ0yDTVxHmm9eL7UpSmY2fIYNdaneuerOgnHwM3J6K2Ov59yJ7SGBNOas/O7?=
 =?us-ascii?Q?54i9R5nPLvVZ55uz+wY8xqKFFCqZCltQJC6i6hLFGPQ6mFhq052nOlDcP2Pl?=
 =?us-ascii?Q?5QA9N0zLJSLcWZ75NRXR+yvNGeDM8/L9ndEvFrdnQqBnvB1UfUgzWacsFFQL?=
 =?us-ascii?Q?tlpg3NlfSdkIAOMB/ndPubDpRVWJ4iTeeJVsRca4VhzkwZYeNEzU6D5pCUrJ?=
 =?us-ascii?Q?AeXmLiZZLk47SkIPm97mKK9MZyJ4bjzEBTth2Lj0VJwFb59ul/kECye18WYM?=
 =?us-ascii?Q?qukWpo5HOycfEvrYynRLudjSrprG0rsvAsp2Xo49QRQaim3LYPIwyoPeApa5?=
 =?us-ascii?Q?vcTAD+4NHZJm67ylMLWG7ssmtwktqqdS8w9GY1v5Me4VUIfVBf1aBrPoFDRY?=
 =?us-ascii?Q?hXaymzPPy3UMUYpk9I2rNXWPot2NwkIr9Oi+27pbbyoBSt6Cb3FiIuuML29+?=
 =?us-ascii?Q?cXczKNXh0an78pI9zPRAcChmVKr27B7EpWD8rUDKj4HZDnxUT/g53c/FsIDv?=
 =?us-ascii?Q?8wK2MqP+NEI1doAbcRplfNHzbl59PGQnaUftjk2syJbLrX91q/AQUpBjcsCG?=
 =?us-ascii?Q?BpEqoYCg6gF9w2dTK93GbxGpVrU2lHmqo6w+tAoSZKFQtdkNLdKE1R2bqJSd?=
 =?us-ascii?Q?If39sZMKNEc26NPZ2g4f/fXsIBxJiUYq1GuwRIVLkkVBW9HfoKSe7GEUiaea?=
 =?us-ascii?Q?BDjDKuIEAwMhPzF8E2vRyiOKn13bfDGGLt1JX7vTF3IhMVQLkM/cTUtbu1k0?=
 =?us-ascii?Q?FhMZ3bIgVws+eMmdM8zZMrHxfPBcKl/cw0A3JuuN6dgmSAFNOZoY+O0Ta0oy?=
 =?us-ascii?Q?9Z7vHj1hazoLhwJWge1bGc6FVlEs7hhMs1FFgKLaeDAGIvB/DGJsBjo8tKV5?=
 =?us-ascii?Q?N+XAneBKk2avKvzeuPcX4lT9k2Yzscfe3cAp2pXZ5jXzahqEFjJwX6EaMOKn?=
 =?us-ascii?Q?fTEa6X1ftc+OklQLkbsOk2zkn0toTMpyYI0vHX4Y3JI9/Eyluh4obwDMOw8K?=
 =?us-ascii?Q?DXzjeg45egi0bDBcZOQIhbHPu6gcGWdbTq7nrInVNuVOapT7hKqUTFFodAyI?=
 =?us-ascii?Q?fJmRwmHjb4K8NORlMyAgeX4CTO2D9TVHo3KVPSDpwQzBpu8WCEj98+BvAXGm?=
 =?us-ascii?Q?D33Yyh6gbXy9dbuzPy4C+m4TGGZHudfxT5ohF+hnsYDy5PBeHIkhYL6Stcae?=
 =?us-ascii?Q?2AH6XV2qWG2aBRmjZo6fgJKx47gtGAN9fR/cO3GZkU1QwvX1u6Zmgt9xbcuf?=
 =?us-ascii?Q?4yTzUtfxu8QLBEDlS/76UgMui/Wp4JySE4loQS15/OaFxumtkyYPI8ttBZZE?=
 =?us-ascii?Q?Bj+1O96YCrLOd6uuQNuRmYFuFepopDGYmSYw3laWJ+YtFijPmKBypGFQ3uf5?=
 =?us-ascii?Q?Xu2IZSUoIhMTuZD8Ob9NzZ1AxuEs9mm7WK33JnAAY4Si3v4PUJOVVYT5orBM?=
 =?us-ascii?Q?+bkMPPUdCHhJHI+ipuj91Fql7+7v87Qp2ix0SWOZ?=
X-OriginatorOrg: zenithal.me
X-MS-Exchange-CrossTenant-Network-Message-Id: afb05ee6-281e-4d17-6649-08dc6d187ccb
X-MS-Exchange-CrossTenant-AuthSource: TYCP286MB1393.JPNP286.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2024 15:31:52.2431
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 436d481c-43b1-4418-8d7f-84c1e4887cf0
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wyKtt/THKLAuXL9DILMS5MvMyBi9ZVesm+/dbrvuMqmTVxh0Zsd/laYCZ2oZWiY5
X-MS-Exchange-Transport-CrossTenantHeadersStamped: OSZP286MB2031

On Sun, Dec 17, 2023 at 08:30:40PM +0100, Simon Holesch wrote:
> Skip submitting URBs, when identical requests were already sent in
> tweak_special_requests(). Instead call the completion handler directly
> to return the result of the URB.

Reproduced the behavior and this patch fixed the bahavior

> 
> Even though submitting those requests twice should be harmless, there
> are USB devices that react poorly to some duplicated requests.
> 
> One example is the ChipIdea controller implementation in U-Boot: The
> second SET_CONFIURATION request makes U-Boot disable and re-enable all
> endpoints. Re-enabling an endpoint in the ChipIdea controller, however,
> was broken until U-Boot commit b272c8792502 ("usb: ci: Fix gadget
> reinit").
> 
> Signed-off-by: Simon Holesch <simon@holesch.de>
> Acked-by: Shuah Khan <skhan@linuxfoundation.org>
> ---

>  /*
> @@ -468,6 +477,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  	int support_sg = 1;
>  	int np = 0;
>  	int ret, i;
> +	int is_tweaked;
>  
>  	if (pipe == -1)
>  		return;
> @@ -580,8 +590,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  		priv->urbs[i]->pipe = pipe;
>  		priv->urbs[i]->complete = stub_complete;
>  
> -		/* no need to submit an intercepted request, but harmless? */
> -		tweak_special_requests(priv->urbs[i]);
> +		is_tweaked = tweak_special_requests(priv->urbs[i]);

One question though, if there are mutiple urbs and one of them is
SET CONFIGURATION, then all of them would not be submitted,
as is_tweaked is a *global* flag instead of a per-urb flag.

Now it is assumed that when the urb is SET CONFIGURATION then
num_urbs is 1. I assume it just happens to be the case and I do
not know if it holds for all scenario.

>  
>  		masking_bogus_flags(priv->urbs[i]);
>  	}
> @@ -594,22 +603,32 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  
>  	/* urb is now ready to submit */
>  	for (i = 0; i < priv->num_urbs; i++) {
> -		ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
> +		if (!is_tweaked) {
> +			ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
>  
> -		if (ret == 0)
> -			usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> -					pdu->base.seqnum);
> -		else {
> -			dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> -			usbip_dump_header(pdu);
> -			usbip_dump_urb(priv->urbs[i]);
> +			if (ret == 0)
> +				usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> +						pdu->base.seqnum);
> +			else {
> +				dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> +				usbip_dump_header(pdu);
> +				usbip_dump_urb(priv->urbs[i]);
>  
> +				/*
> +				 * Pessimistic.
> +				 * This connection will be discarded.
> +				 */
> +				usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> +				break;
> +			}
> +		} else {
>  			/*
> -			 * Pessimistic.
> -			 * This connection will be discarded.
> +			 * An identical URB was already submitted in
> +			 * tweak_special_requests(). Skip submitting this URB to not
> +			 * duplicate the request.
>  			 */
> -			usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> -			break;
> +			priv->urbs[i]->status = 0;
> +			stub_complete(priv->urbs[i]);
>  		}
>  	}
>  
> -- 
> 2.43.0
>