Date: Mon, 11 Mar 2024 13:04:18 +0100
From: Hardik Gajjar <hgajjar@...adit-jv.com>
To: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>,
	<gregkh@...uxfoundation.org>
CC: Hardik Gajjar <hgajjar@...adit-jv.com>, <maze@...gle.com>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<guofeng.li@...com>, <hardik.gajjar@...ch.com>, <eugeniu.rosca@...ch.com>
Subject: Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of
 invalid gadget ptr

On Fri, Mar 08, 2024 at 05:47:37PM +0530, Krishna Kurapati PSSNV wrote:
> 
> 
> On 3/8/2024 5:25 PM, Hardik Gajjar wrote:
> > On Thu, Mar 07, 2024 at 11:12:07PM +0530, Krishna Kurapati PSSNV wrote:
> > > 
> 
> [...]
> 
> > 
> > I believe using gether_cleanup altogether may not be an optimal solution.
> > The creation and deletion of network interfaces should align with the behavior of each specific network driver.
> > 
> > For instance, in the case of NCM, the usb0 interface is created upon the creation of a directory
> > in config/usb_gadget/gX/functions/ncm.usb0, and it is removed when the corresponding directory
> > is deleted. This follows a standard flow observed in many network drivers, where interfaces are
> > created during driver loading/probing and deleted during removal.
> > 
> 
> Hi Hardik,
> 
>  Yeah. I observed this behavior.
> 
> > Typically, deleting the gadget on every disconnection is not considered a good practice, as it can
> > negatively impact the user experience when accessing the gadget.
> > 
> 
> Got it. I was suspecting issues might come up during fast Plug-In/ Plug-Out
> cases as setting and cleanup of network interface might take some time.
> 
> > In our specific scenario, retaining the usb0 network interface has proven to enhance performance
> > and stabilize connections. Previous attempts to remove it resulted in an observed increase in time of 300ms,
> > particularly at the start of Apple CarPlay sessions.
> > 
> 
> Thanks for this info. Makes sense to keep it in free_inst only. But my
> initial doubt only stemmed from the fact that actions taken during bind must
> be reversed during unbind.
> 
> > Furthermore, it's important to highlight that in Qualcomm products and msm kernels, the inclusion of gether_cleanup
> > in the unbind process was eventually reverted. While the specific reason for reverting the patch is unknown,
> > it suggests that the addition may not have yielded the intended or required results
> > 
> > Following is the revert patch details in msm-5.4 kernel, if you want check it.
> > 
> > Revert "usb: gadget: f_ncm: allocate/free net device upon driver bind/unbind"
> > 
> > This reverts commit 006d8adf555a8c6d34113f564ade312d68abd3b3.
> > 
> > Move back the allocation of netdevice to alloc_inst(), one-time
> > registration to bind(), deregistration and free to rm_inst(). The
> > UI update issue will be taken up with proper stakeholders.
> > 
> > Change-Id: I56448b08f6796a43ec5b0dfe0dd2d42cdc0eec14
> > 
> 
> Agree. This was merged first in 4.19 downstream (most probably for car play
> use case only) and then reverted in 5.4. But present 4.19 code in QC still
> keeps it in unbind path. The only reason I suspect it was reverted was to
> get back on to same page with upstream driver (atleast starting from 5.10,
> this was the reasoning to remove gether_cleanup in unbind path and put it
> back in free_inst).
> 
> Thanks,
> Krishna,

Thanks Krinshna for your comment
Come to the first comment from Greg. 

> What commit id does this fix?
> 
> thanks,
> 
> greg k-h

>Hi Greg,

>The network device parent is not being properly cleaned up during unbind since the \
>initial commit of the driver. In that case, should I mention the First commit ID of \
>the driver as broken commit or would you advice to say, For example "Clean up netdev \
>parent in unbind".

>Thanks,
>Hardik

@Greg, 

Can you please provide guidance on how to proceed? Should I update the commit message to exclude the term 'Fix' in the title?

Thanks,
Hardik

Powered by blists - more mailing lists