| Message ID | 5c923258-67c3-bae1-80d1-87a187202a4c@omp.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: common: usb-otg-fsm: drop unreachable code in otg_statemachine() | expand |
On Tue, Feb 08, 2022 at 11:32:28PM +0300, Sergey Shtylyov wrote: > The *switch* statement in otg_statemachine() does handle all possible OTG > states explicitly, so the *default* label is unreachable. > > Found by Linux Verification Center (linuxtesting.org) with the SVACE static > analysis tool. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo. > Peter Chen's 'usb.git' repo seems outdated, so I chose to ignore it... > > drivers/usb/common/usb-otg-fsm.c | 2 -- > 1 file changed, 2 deletions(-) > > Index: usb/drivers/usb/common/usb-otg-fsm.c > =================================================================== > --- usb.orig/drivers/usb/common/usb-otg-fsm.c > +++ usb/drivers/usb/common/usb-otg-fsm.c > @@ -440,8 +440,6 @@ int otg_statemachine(struct otg_fsm *fsm > if (fsm->id || fsm->a_bus_drop || fsm->a_clr_err) > otg_set_state(fsm, OTG_STATE_A_WAIT_VFALL); > break; > - default: > - break; > } > mutex_unlock(&fsm->lock); > There is nothing wrong with leaving lines like this in the code to handle any potential bugs. Why do you think it needs to be removed? What benefit does this patch have?
On 2/9/22 8:35 AM, Greg Kroah-Hartman wrote: >> The *switch* statement in otg_statemachine() does handle all possible OTG >> states explicitly, so the *default* label is unreachable. >> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static >> analysis tool. >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> >> --- >> This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo. >> Peter Chen's 'usb.git' repo seems outdated, so I chose to ignore it... >> >> drivers/usb/common/usb-otg-fsm.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> Index: usb/drivers/usb/common/usb-otg-fsm.c >> =================================================================== >> --- usb.orig/drivers/usb/common/usb-otg-fsm.c >> +++ usb/drivers/usb/common/usb-otg-fsm.c >> @@ -440,8 +440,6 @@ int otg_statemachine(struct otg_fsm *fsm >> if (fsm->id || fsm->a_bus_drop || fsm->a_clr_err) >> otg_set_state(fsm, OTG_STATE_A_WAIT_VFALL); >> break; >> - default: >> - break; >> } >> mutex_unlock(&fsm->lock); >> > > There is nothing wrong with leaving lines like this in the code to > handle any potential bugs. > Why do you think it needs to be removed? What benefit does this patch > have? These lines as they are bring no value at all. Note that (as I said) all the values of 'enum usb_otg_state' are already handled with explicit *case* label... MBR, Sergey
On Wed, Feb 09, 2022 at 01:33:53PM +0300, Sergey Shtylyov wrote: > On 2/9/22 8:35 AM, Greg Kroah-Hartman wrote: > > >> The *switch* statement in otg_statemachine() does handle all possible OTG > >> states explicitly, so the *default* label is unreachable. > >> > >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static > >> analysis tool. > >> > >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > >> > >> --- > >> This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo. > >> Peter Chen's 'usb.git' repo seems outdated, so I chose to ignore it... > >> > >> drivers/usb/common/usb-otg-fsm.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> Index: usb/drivers/usb/common/usb-otg-fsm.c > >> =================================================================== > >> --- usb.orig/drivers/usb/common/usb-otg-fsm.c > >> +++ usb/drivers/usb/common/usb-otg-fsm.c > >> @@ -440,8 +440,6 @@ int otg_statemachine(struct otg_fsm *fsm > >> if (fsm->id || fsm->a_bus_drop || fsm->a_clr_err) > >> otg_set_state(fsm, OTG_STATE_A_WAIT_VFALL); > >> break; > >> - default: > >> - break; > >> } > >> mutex_unlock(&fsm->lock); > >> > > > > There is nothing wrong with leaving lines like this in the code to > > handle any potential bugs. > > Why do you think it needs to be removed? What benefit does this patch > > have? > > These lines as they are bring no value at all. > Note that (as I said) all the values of 'enum usb_otg_state' are > already handled with explicit *case* label... And so now you will trigger the checkers that ask "you do not have a default: line for your case statement!" This is safer as is, please do not "clean up" things that are not actually a problem. thanks, greg k-h
Index: usb/drivers/usb/common/usb-otg-fsm.c =================================================================== --- usb.orig/drivers/usb/common/usb-otg-fsm.c +++ usb/drivers/usb/common/usb-otg-fsm.c @@ -440,8 +440,6 @@ int otg_statemachine(struct otg_fsm *fsm if (fsm->id || fsm->a_bus_drop || fsm->a_clr_err) otg_set_state(fsm, OTG_STATE_A_WAIT_VFALL); break; - default: - break; } mutex_unlock(&fsm->lock);
The *switch* statement in otg_statemachine() does handle all possible OTG states explicitly, so the *default* label is unreachable. Found by Linux Verification Center (linuxtesting.org) with the SVACE static analysis tool. Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo. Peter Chen's 'usb.git' repo seems outdated, so I chose to ignore it... drivers/usb/common/usb-otg-fsm.c | 2 -- 1 file changed, 2 deletions(-)