Hi Greg,
>
> On Wed, Jan 24, 2024 at 02:45:54PM +0800, Xu Yang wrote:
> > There is a possibility that usb_role_switch device is unregistered before
> > the user put usb_role_switch. In this case, the user may still want to
> > get/set_role() since the user can't sense the changes of usb_role_switch.
> >
> > This will add a flag to show if usb_role_switch is already registered and
> > avoid unwanted behaviors.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> >
> > ---
> > Changes in v2:
> > - new patch during test patch 1
> > - add registered flag
> > ---
> > drivers/usb/roles/class.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index 2bad038fb9ad..70165dd86b5d 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -23,6 +23,7 @@ struct usb_role_switch {
> > struct mutex lock; /* device lock*/
> > struct module *module; /* the module this device depends on */
> > enum usb_role role;
> > + bool registered;
> >
> > /* From descriptor */
> > struct device *usb2_port;
> > @@ -49,6 +50,9 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
> > if (IS_ERR_OR_NULL(sw))
> > return 0;
> >
> > + if (!sw->registered)
> > + return -EOPNOTSUPP;
>
> What's to prevent this from changing right after you check it?
Usually , the usb_role_switch device is unregistered after usb controller
device is removed.
Such as dwc3 platform, if the usb controller is in device mode at first,
dwc3_gadget_exit() will be called when removing dwc3 controller device
by unbind the device. If typec port changes to DFP, tcpm will set usb role
to host mode and usb_role_switch_set_role() is called. Then dwc3 controller
driver will call dwc3_gadget_exit() again to switch from gadget to host mode.
But this time kernel will dump NULL pointer issue since gadget resource is
already released.
[ 46.065015] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
[ 46.074030] Mem abort info:
[ 46.076915] ESR = 0x0000000096000045
[ 46.080742] EC = 0x25: DABT (current EL), IL = 32 bits
[ 46.086155] SET = 0, FnV = 0
...
[ 46.272542] Call trace:
[ 46.274986] usb_del_gadget+0x44/0xac
[ 46.278651] dwc3_gadget_exit+0x20/0x7c
[ 46.282490] __dwc3_set_mode+0x280/0x3ec
[ 46.286408] process_one_work+0x138/0x248
[ 46.290421] worker_thread+0x320/0x438
[ 46.294173] kthread+0x110/0x114
[ 46.297406] ret_from_fork+0x10/0x20
[ 46.300992] Code: f94186a4 d2802002 f9418aa3 f2fbd5a2 (f9000483)
[ 46.307079] ---[ end trace 0000000000000000 ]---
In chipidea platform, I also get below kernel dump.
[ 78.499672] Unable to handle kernel paging request at virtual address ffff8000822a51a4
[ 78.507588] Mem abort info:
[ 78.510366] ESR = 0x0000000096000007
[ 78.514102] EC = 0x25: DABT (current EL), IL = 32 bits
[ 78.519397] SET = 0, FnV = 0
...
[ 78.705713] Call trace:
[ 78.708149] hw_read_otgsc+0x8/0xf4
[ 78.711632] ci_usb_role_switch_set+0x94/0x2c4
[ 78.716069] usb_role_switch_set_role+0x44/0x98
[ 78.720593] tcpm_mux_set+0x5c/0x80
[ 78.724069] tcpm_set_roles+0x64/0xd4
[ 78.727717] tcpm_state_machine_work+0x2350/0x2ff4
[ 78.732502] kthread_worker_fn+0xc4/0x174
[ 78.736506] kthread+0x110/0x114
[ 78.739721] ret_from_fork+0x10/0x20
[ 78.743295] Code: 88dffc21 88dffc00 f9405c02 aa0003e3 (b9400042)
[ 78.749377] ---[ end trace 0000000000000000 ]---
Maybe these platforms lack some checks about the resources. But, first of all,
->set_role() should not be called at all when usb_role_switch device is
Unregistered.
>
> And why is this patch not cc: stable and have a fixes tag if it resolves
> a real issue for people?
Sorry, I forget that. Will add it in next version.
Thanks,
Xu Yang
