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

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

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,