* [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr @ 2024-03-07 16:18 Hardik Gajjar 2024-03-07 16:45 ` Greg KH 2024-03-07 17:42 ` Krishna Kurapati PSSNV 0 siblings, 2 replies; 8+ messages in thread From: Hardik Gajjar @ 2024-03-07 16:18 UTC (permalink / raw) To: gregkh, quic_kriskura, maze, quic_linyyuan Cc: linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca In the scenario where the system enters suspend to RAM mode (STR) triggers the disconnection of Dual Role USB Hub, and the UDC platform driver calls usb_del_gadget_udc() to cleanup and delete the associated gadget. However, at this point, the usb0 interface is not yet deleted, leading to a race condition with the TCP/IP stack attempting to access the network device parent (gadget pointer), through operations like the GETLINK net message. This patch addresses the issue by clearing the netdevice's parent device pointer when the ncm unbinds, effectively preventing the race condition during this critical phase. Followinfg is the backtrace of such race condition [ 3566.105792] Call trace: [ 3566.105984] if_nlmsg_size+0x48/0x3b0 [ 3566.107497] rtnetlink_rcv_msg+0x1cc/0x408 [ 3566.107905] netlink_rcv_skb+0x12c/0x164 [ 3566.108264] rtnetlink_rcv+0x18/0x24 [ 3566.108851] netlink_unicast_kernel+0xc4/0x14c [ 3566.109192] netlink_unicast+0x210/0x2b0 [ 3566.109606] netlink_sendmsg+0x2ec/0x360 [ 3566.110046] __sys_sendto+0x1b8/0x25c [ 3566.111594] __arm64_sys_sendto+0x28/0x38 [ 3566.112599] el0_svc_common+0xb4/0x19c [ 3566.112978] el0_svc_handler+0x74/0x98 [ 3566.113269] el0_svc+0x8/0xc - code: if_nlmsg_size call the following function static inline int rtnl_vfinfo_size(const struct net_device *dev, u32 ext_filter_mask) { // dev->dev.parent is not NULL if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF)) { // dev_num_vf use the dev->dev.parent->bus lead to kernel panic. int num_vfs = dev_num_vf(dev->dev.parent); size_t size = nla_total_size(0); size += num_vfs * (nla_total_size(0) + nla_total_size(sizeof(struct ifla_vf_mac)) + nla_total_size(sizeof(struct ifla_vf_vlan)) + nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST * Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> --- drivers/usb/gadget/function/f_ncm.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index e2a059cfda2c..fdfb5b3460c7 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -1728,9 +1728,12 @@ static void ncm_free(struct usb_function *f) static void ncm_unbind(struct usb_configuration *c, struct usb_function *f) { struct f_ncm *ncm = func_to_ncm(f); + struct f_ncm_opts *ncm_opts; DBG(c->cdev, "ncm unbind\n"); + ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst); + hrtimer_cancel(&ncm->task_timer); kfree(f->os_desc_table); @@ -1746,6 +1749,10 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f) kfree(ncm->notify_req->buf); usb_ep_free_request(ncm->notify, ncm->notify_req); + + mutex_lock(&ncm_opts->lock); + SET_NETDEV_DEV(ncm_opts->net, NULL); + mutex_unlock(&ncm_opts->lock); } static struct usb_function *ncm_alloc(struct usb_function_instance *fi) -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr 2024-03-07 16:18 [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr Hardik Gajjar @ 2024-03-07 16:45 ` Greg KH 2024-03-07 17:33 ` Hardik Gajjar 2024-03-07 17:42 ` Krishna Kurapati PSSNV 1 sibling, 1 reply; 8+ messages in thread From: Greg KH @ 2024-03-07 16:45 UTC (permalink / raw) To: Hardik Gajjar Cc: quic_kriskura, maze, quic_linyyuan, linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca On Thu, Mar 07, 2024 at 05:18:49PM +0100, Hardik Gajjar wrote: > In the scenario where the system enters suspend to RAM mode (STR) triggers > the disconnection of Dual Role USB Hub, and the UDC platform driver calls > usb_del_gadget_udc() to cleanup and delete the associated gadget. > > However, at this point, the usb0 interface is not yet deleted, leading to > a race condition with the TCP/IP stack attempting to access the network > device parent (gadget pointer), through operations like the GETLINK net > message. > > This patch addresses the issue by clearing the netdevice's parent device > pointer when the ncm unbinds, effectively preventing the race condition > during this critical phase. > > Followinfg is the backtrace of such race condition > [ 3566.105792] Call trace: > [ 3566.105984] if_nlmsg_size+0x48/0x3b0 > [ 3566.107497] rtnetlink_rcv_msg+0x1cc/0x408 > [ 3566.107905] netlink_rcv_skb+0x12c/0x164 > [ 3566.108264] rtnetlink_rcv+0x18/0x24 > [ 3566.108851] netlink_unicast_kernel+0xc4/0x14c > [ 3566.109192] netlink_unicast+0x210/0x2b0 > [ 3566.109606] netlink_sendmsg+0x2ec/0x360 > [ 3566.110046] __sys_sendto+0x1b8/0x25c > [ 3566.111594] __arm64_sys_sendto+0x28/0x38 > [ 3566.112599] el0_svc_common+0xb4/0x19c > [ 3566.112978] el0_svc_handler+0x74/0x98 > [ 3566.113269] el0_svc+0x8/0xc > > - code: if_nlmsg_size call the following function > > static inline int rtnl_vfinfo_size(const struct net_device *dev, > u32 ext_filter_mask) > { > // dev->dev.parent is not NULL > if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF)) { > // dev_num_vf use the dev->dev.parent->bus lead to kernel panic. > int num_vfs = dev_num_vf(dev->dev.parent); > size_t size = nla_total_size(0); > size += num_vfs * > (nla_total_size(0) + > nla_total_size(sizeof(struct ifla_vf_mac)) + > nla_total_size(sizeof(struct ifla_vf_vlan)) + > nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST * > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > --- > drivers/usb/gadget/function/f_ncm.c | 7 +++++++ > 1 file changed, 7 insertions(+) What commit id does this fix? thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr 2024-03-07 16:45 ` Greg KH @ 2024-03-07 17:33 ` Hardik Gajjar 0 siblings, 0 replies; 8+ messages in thread From: Hardik Gajjar @ 2024-03-07 17:33 UTC (permalink / raw) To: Greg KH Cc: Hardik Gajjar, quic_kriskura, maze, quic_linyyuan, linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca On Thu, Mar 07, 2024 at 04:45:13PM +0000, Greg KH wrote: > On Thu, Mar 07, 2024 at 05:18:49PM +0100, Hardik Gajjar wrote: > > In the scenario where the system enters suspend to RAM mode (STR) triggers > > the disconnection of Dual Role USB Hub, and the UDC platform driver calls > > usb_del_gadget_udc() to cleanup and delete the associated gadget. > > > > However, at this point, the usb0 interface is not yet deleted, leading to > > a race condition with the TCP/IP stack attempting to access the network > > device parent (gadget pointer), through operations like the GETLINK net > > message. > > > > This patch addresses the issue by clearing the netdevice's parent device > > pointer when the ncm unbinds, effectively preventing the race condition > > during this critical phase. > > > > Followinfg is the backtrace of such race condition > > [ 3566.105792] Call trace: > > [ 3566.105984] if_nlmsg_size+0x48/0x3b0 > > [ 3566.107497] rtnetlink_rcv_msg+0x1cc/0x408 > > [ 3566.107905] netlink_rcv_skb+0x12c/0x164 > > [ 3566.108264] rtnetlink_rcv+0x18/0x24 > > [ 3566.108851] netlink_unicast_kernel+0xc4/0x14c > > [ 3566.109192] netlink_unicast+0x210/0x2b0 > > [ 3566.109606] netlink_sendmsg+0x2ec/0x360 > > [ 3566.110046] __sys_sendto+0x1b8/0x25c > > [ 3566.111594] __arm64_sys_sendto+0x28/0x38 > > [ 3566.112599] el0_svc_common+0xb4/0x19c > > [ 3566.112978] el0_svc_handler+0x74/0x98 > > [ 3566.113269] el0_svc+0x8/0xc > > > > - code: if_nlmsg_size call the following function > > > > static inline int rtnl_vfinfo_size(const struct net_device *dev, > > u32 ext_filter_mask) > > { > > // dev->dev.parent is not NULL > > if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF)) { > > // dev_num_vf use the dev->dev.parent->bus lead to kernel panic. > > int num_vfs = dev_num_vf(dev->dev.parent); > > size_t size = nla_total_size(0); > > size += num_vfs * > > (nla_total_size(0) + > > nla_total_size(sizeof(struct ifla_vf_mac)) + > > nla_total_size(sizeof(struct ifla_vf_vlan)) + > > nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST * > > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > > --- > > drivers/usb/gadget/function/f_ncm.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr 2024-03-07 16:18 [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr Hardik Gajjar 2024-03-07 16:45 ` Greg KH @ 2024-03-07 17:42 ` Krishna Kurapati PSSNV 2024-03-08 11:55 ` Hardik Gajjar 1 sibling, 1 reply; 8+ messages in thread From: Krishna Kurapati PSSNV @ 2024-03-07 17:42 UTC (permalink / raw) To: Hardik Gajjar, gregkh, maze Cc: linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca On 3/7/2024 9:48 PM, Hardik Gajjar wrote: > In the scenario where the system enters suspend to RAM mode (STR) triggers > the disconnection of Dual Role USB Hub, and the UDC platform driver calls > usb_del_gadget_udc() to cleanup and delete the associated gadget. > > However, at this point, the usb0 interface is not yet deleted, leading to > a race condition with the TCP/IP stack attempting to access the network > device parent (gadget pointer), through operations like the GETLINK net > message. > > This patch addresses the issue by clearing the netdevice's parent device > pointer when the ncm unbinds, effectively preventing the race condition > during this critical phase. > Hi Hardik, Would this the case be same with other network functions as well ? I see that for all gadget functions, the network interface exists although the unbind is done and deleted only upon function instance removal. Should we move the gether_cleanup at unbind as a reverse operation of what we do on first bind ? If we do so, I think this current problem too would be gone. Greg, if you have idea why we don't destroy the network interface upon unbind and keep it till the lifetime of the function instance, can you help with that info. I was trying to see if we can move the gether_cleanup call to unbind, but I don't know why it was kept in free_inst to begin with, so I didn't touch that part of code so far. Regards, Krishna, > Followinfg is the backtrace of such race condition > [ 3566.105792] Call trace: > [ 3566.105984] if_nlmsg_size+0x48/0x3b0 > [ 3566.107497] rtnetlink_rcv_msg+0x1cc/0x408 > [ 3566.107905] netlink_rcv_skb+0x12c/0x164 > [ 3566.108264] rtnetlink_rcv+0x18/0x24 > [ 3566.108851] netlink_unicast_kernel+0xc4/0x14c > [ 3566.109192] netlink_unicast+0x210/0x2b0 > [ 3566.109606] netlink_sendmsg+0x2ec/0x360 > [ 3566.110046] __sys_sendto+0x1b8/0x25c > [ 3566.111594] __arm64_sys_sendto+0x28/0x38 > [ 3566.112599] el0_svc_common+0xb4/0x19c > [ 3566.112978] el0_svc_handler+0x74/0x98 > [ 3566.113269] el0_svc+0x8/0xc > > - code: if_nlmsg_size call the following function > > static inline int rtnl_vfinfo_size(const struct net_device *dev, > u32 ext_filter_mask) > { > // dev->dev.parent is not NULL > if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF)) { > // dev_num_vf use the dev->dev.parent->bus lead to kernel panic. > int num_vfs = dev_num_vf(dev->dev.parent); > size_t size = nla_total_size(0); > size += num_vfs * > (nla_total_size(0) + > nla_total_size(sizeof(struct ifla_vf_mac)) + > nla_total_size(sizeof(struct ifla_vf_vlan)) + > nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST * > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > --- > drivers/usb/gadget/function/f_ncm.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c > index e2a059cfda2c..fdfb5b3460c7 100644 > --- a/drivers/usb/gadget/function/f_ncm.c > +++ b/drivers/usb/gadget/function/f_ncm.c > @@ -1728,9 +1728,12 @@ static void ncm_free(struct usb_function *f) > static void ncm_unbind(struct usb_configuration *c, struct usb_function *f) > { > struct f_ncm *ncm = func_to_ncm(f); > + struct f_ncm_opts *ncm_opts; > > DBG(c->cdev, "ncm unbind\n"); > > + ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst); > + > hrtimer_cancel(&ncm->task_timer); > > kfree(f->os_desc_table); > @@ -1746,6 +1749,10 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f) > > kfree(ncm->notify_req->buf); > usb_ep_free_request(ncm->notify, ncm->notify_req); > + > + mutex_lock(&ncm_opts->lock); > + SET_NETDEV_DEV(ncm_opts->net, NULL); > + mutex_unlock(&ncm_opts->lock); > } > > static struct usb_function *ncm_alloc(struct usb_function_instance *fi) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr 2024-03-07 17:42 ` Krishna Kurapati PSSNV @ 2024-03-08 11:55 ` Hardik Gajjar 2024-03-08 12:17 ` Krishna Kurapati PSSNV 0 siblings, 1 reply; 8+ messages in thread From: Hardik Gajjar @ 2024-03-08 11:55 UTC (permalink / raw) To: Krishna Kurapati PSSNV Cc: Hardik Gajjar, gregkh, maze, linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca On Thu, Mar 07, 2024 at 11:12:07PM +0530, Krishna Kurapati PSSNV wrote: > > On 3/7/2024 9:48 PM, Hardik Gajjar wrote: > > In the scenario where the system enters suspend to RAM mode (STR) triggers > > the disconnection of Dual Role USB Hub, and the UDC platform driver calls > > usb_del_gadget_udc() to cleanup and delete the associated gadget. > > > > However, at this point, the usb0 interface is not yet deleted, leading to > > a race condition with the TCP/IP stack attempting to access the network > > device parent (gadget pointer), through operations like the GETLINK net > > message. > > > > This patch addresses the issue by clearing the netdevice's parent device > > pointer when the ncm unbinds, effectively preventing the race condition > > during this critical phase. > > > > Hi Hardik, > > Would this the case be same with other network functions as well ? I see > that for all gadget functions, the network interface exists although the > unbind is done and deleted only upon function instance removal. Should we > move the gether_cleanup at unbind as a reverse operation of what we do on > first bind ? If we do so, I think this current problem too would be gone. > > Greg, if you have idea why we don't destroy the network interface upon > unbind and keep it till the lifetime of the function instance, can you help > with that info. I was trying to see if we can move the gether_cleanup call > to unbind, but I don't know why it was kept in free_inst to begin with, so I > didn't touch that part of code so far. > > Regards, > Krishna, Hi Krishna, 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. 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. 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. 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 Thanks, Hardik > > > Followinfg is the backtrace of such race condition > > [ 3566.105792] Call trace: > > [ 3566.105984] if_nlmsg_size+0x48/0x3b0 > > [ 3566.107497] rtnetlink_rcv_msg+0x1cc/0x408 > > [ 3566.107905] netlink_rcv_skb+0x12c/0x164 > > [ 3566.108264] rtnetlink_rcv+0x18/0x24 > > [ 3566.108851] netlink_unicast_kernel+0xc4/0x14c > > [ 3566.109192] netlink_unicast+0x210/0x2b0 > > [ 3566.109606] netlink_sendmsg+0x2ec/0x360 > > [ 3566.110046] __sys_sendto+0x1b8/0x25c > > [ 3566.111594] __arm64_sys_sendto+0x28/0x38 > > [ 3566.112599] el0_svc_common+0xb4/0x19c > > [ 3566.112978] el0_svc_handler+0x74/0x98 > > [ 3566.113269] el0_svc+0x8/0xc > > > > - code: if_nlmsg_size call the following function > > > > static inline int rtnl_vfinfo_size(const struct net_device *dev, > > u32 ext_filter_mask) > > { > > // dev->dev.parent is not NULL > > if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF)) { > > // dev_num_vf use the dev->dev.parent->bus lead to kernel panic. > > int num_vfs = dev_num_vf(dev->dev.parent); > > size_t size = nla_total_size(0); > > size += num_vfs * > > (nla_total_size(0) + > > nla_total_size(sizeof(struct ifla_vf_mac)) + > > nla_total_size(sizeof(struct ifla_vf_vlan)) + > > nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST * > > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > > --- > > drivers/usb/gadget/function/f_ncm.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c > > index e2a059cfda2c..fdfb5b3460c7 100644 > > --- a/drivers/usb/gadget/function/f_ncm.c > > +++ b/drivers/usb/gadget/function/f_ncm.c > > @@ -1728,9 +1728,12 @@ static void ncm_free(struct usb_function *f) > > static void ncm_unbind(struct usb_configuration *c, struct usb_function *f) > > { > > struct f_ncm *ncm = func_to_ncm(f); > > + struct f_ncm_opts *ncm_opts; > > DBG(c->cdev, "ncm unbind\n"); > > + ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst); > > + > > hrtimer_cancel(&ncm->task_timer); > > kfree(f->os_desc_table); > > @@ -1746,6 +1749,10 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f) > > kfree(ncm->notify_req->buf); > > usb_ep_free_request(ncm->notify, ncm->notify_req); > > + > > + mutex_lock(&ncm_opts->lock); > > + SET_NETDEV_DEV(ncm_opts->net, NULL); > > + mutex_unlock(&ncm_opts->lock); > > } > > static struct usb_function *ncm_alloc(struct usb_function_instance *fi) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr 2024-03-08 11:55 ` Hardik Gajjar @ 2024-03-08 12:17 ` Krishna Kurapati PSSNV 2024-03-11 12:04 ` Hardik Gajjar 0 siblings, 1 reply; 8+ messages in thread From: Krishna Kurapati PSSNV @ 2024-03-08 12:17 UTC (permalink / raw) To: Hardik Gajjar Cc: gregkh, maze, linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca 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, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr 2024-03-08 12:17 ` Krishna Kurapati PSSNV @ 2024-03-11 12:04 ` Hardik Gajjar 2024-04-19 13:35 ` Hardik Gajjar 0 siblings, 1 reply; 8+ messages in thread From: Hardik Gajjar @ 2024-03-11 12:04 UTC (permalink / raw) To: Krishna Kurapati PSSNV, gregkh Cc: Hardik Gajjar, maze, linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr 2024-03-11 12:04 ` Hardik Gajjar @ 2024-04-19 13:35 ` Hardik Gajjar 0 siblings, 0 replies; 8+ messages in thread From: Hardik Gajjar @ 2024-04-19 13:35 UTC (permalink / raw) To: Krishna Kurapati PSSNV, gregkh Cc: Hardik Gajjar, maze, linux-usb, linux-kernel, guofeng.li, hardik.gajjar, eugeniu.rosca 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-19 13:35 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-07 16:18 [PATCH] usb: gadget: f_ncm: Fix Kernel Panic due to access of invalid gadget ptr Hardik Gajjar 2024-03-07 16:45 ` Greg KH 2024-03-07 17:33 ` Hardik Gajjar 2024-03-07 17:42 ` Krishna Kurapati PSSNV 2024-03-08 11:55 ` Hardik Gajjar 2024-03-08 12:17 ` Krishna Kurapati PSSNV 2024-03-11 12:04 ` Hardik Gajjar 2024-04-19 13:35 ` Hardik Gajjar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).