Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr

[Hardik Gajjar]

On Mon, Mar 11, 2024 at 01:04:18PM +0100, Hardik Gajjar wrote:
> 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

Hi Greg,

Should we move forward if there are no further comments?

Thanks,
Hardik