RE: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void
Hi, "Du, Changbin"writes: >> >root = debugfs_create_dir(dev_name(dwc->dev), NULL); >> > - if (!root) { >> > - ret = -ENOMEM; >> > - goto err0; >> > - } >> > + if (IS_ERR_OR_NULL(root)) >> > + return; >> >> We can definitely keep on going, but I'd still like to know that we >> enabled CONFIG_DEBUG_FS but failed to create a file or a >> directory. Seems like this should read as follows: >> >> if (IS_ERR_OR_NULL(root)) { >> if (!root) >> dev_err(dwc->dev, "Can't create debugfs root\n"); >> return; >> } >> >> ditto to all bellow. >> > Balbi, so you mean we should not let the failure go silent, right? yeah, but only iff CONFIG_DEBUG_FS is enabled. From what I can tell, in case CONFIG_DEBUG_FS is enabled and it fails, it'll return a NULL pointer instead of ERR_PTR() ;-) >> >dwc->root = root; >> > >> >dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL); >> >if (!dwc->regset) { >> > - ret = -ENOMEM; >> > - goto err1; >> > + debugfs_remove_recursive(root); >> >> you're now duplicating debugfs_remove_recursive(root) in all braches and >> that's error prone. It's probably better to keep our gotos, but change >> them so they read as follows: >> >> if (!dwc->regset) >> goto err1; >> >> [...] >> >> return; /* this is our successful exit point */ >> >> err1: >> debugfs_remove_recursive(root); >> kfree(dwc->regset); >> >> >> -- >> Balbi > > No, no need anymore. Because no branch share this code now. > Then remove the goto would make code a little clear. fair enough. -- balbi signature.asc Description: PGP signature
RE: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void
Hi, "Du, Changbin" writes: >> >root = debugfs_create_dir(dev_name(dwc->dev), NULL); >> > - if (!root) { >> > - ret = -ENOMEM; >> > - goto err0; >> > - } >> > + if (IS_ERR_OR_NULL(root)) >> > + return; >> >> We can definitely keep on going, but I'd still like to know that we >> enabled CONFIG_DEBUG_FS but failed to create a file or a >> directory. Seems like this should read as follows: >> >> if (IS_ERR_OR_NULL(root)) { >> if (!root) >> dev_err(dwc->dev, "Can't create debugfs root\n"); >> return; >> } >> >> ditto to all bellow. >> > Balbi, so you mean we should not let the failure go silent, right? yeah, but only iff CONFIG_DEBUG_FS is enabled. From what I can tell, in case CONFIG_DEBUG_FS is enabled and it fails, it'll return a NULL pointer instead of ERR_PTR() ;-) >> >dwc->root = root; >> > >> >dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL); >> >if (!dwc->regset) { >> > - ret = -ENOMEM; >> > - goto err1; >> > + debugfs_remove_recursive(root); >> >> you're now duplicating debugfs_remove_recursive(root) in all braches and >> that's error prone. It's probably better to keep our gotos, but change >> them so they read as follows: >> >> if (!dwc->regset) >> goto err1; >> >> [...] >> >> return; /* this is our successful exit point */ >> >> err1: >> debugfs_remove_recursive(root); >> kfree(dwc->regset); >> >> >> -- >> Balbi > > No, no need anymore. Because no branch share this code now. > Then remove the goto would make code a little clear. fair enough. -- balbi signature.asc Description: PGP signature
RE: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void
> > root = debugfs_create_dir(dev_name(dwc->dev), NULL); > > - if (!root) { > > - ret = -ENOMEM; > > - goto err0; > > - } > > + if (IS_ERR_OR_NULL(root)) > > + return; > > We can definitely keep on going, but I'd still like to know that we > enabled CONFIG_DEBUG_FS but failed to create a file or a > directory. Seems like this should read as follows: > > if (IS_ERR_OR_NULL(root)) { > if (!root) > dev_err(dwc->dev, "Can't create debugfs root\n"); > return; > } > > ditto to all bellow. > Balbi, so you mean we should not let the failure go silent, right? > > dwc->root = root; > > > > dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL); > > if (!dwc->regset) { > > - ret = -ENOMEM; > > - goto err1; > > + debugfs_remove_recursive(root); > > you're now duplicating debugfs_remove_recursive(root) in all braches and > that's error prone. It's probably better to keep our gotos, but change > them so they read as follows: > > if (!dwc->regset) > goto err1; > > [...] > > return; /* this is our successful exit point */ > > err1: > debugfs_remove_recursive(root); > kfree(dwc->regset); > > > -- > Balbi No, no need anymore. Because no branch share this code now. Then remove the goto would make code a little clear. Thanks, Du, Changbin
RE: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void
> > root = debugfs_create_dir(dev_name(dwc->dev), NULL); > > - if (!root) { > > - ret = -ENOMEM; > > - goto err0; > > - } > > + if (IS_ERR_OR_NULL(root)) > > + return; > > We can definitely keep on going, but I'd still like to know that we > enabled CONFIG_DEBUG_FS but failed to create a file or a > directory. Seems like this should read as follows: > > if (IS_ERR_OR_NULL(root)) { > if (!root) > dev_err(dwc->dev, "Can't create debugfs root\n"); > return; > } > > ditto to all bellow. > Balbi, so you mean we should not let the failure go silent, right? > > dwc->root = root; > > > > dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL); > > if (!dwc->regset) { > > - ret = -ENOMEM; > > - goto err1; > > + debugfs_remove_recursive(root); > > you're now duplicating debugfs_remove_recursive(root) in all braches and > that's error prone. It's probably better to keep our gotos, but change > them so they read as follows: > > if (!dwc->regset) > goto err1; > > [...] > > return; /* this is our successful exit point */ > > err1: > debugfs_remove_recursive(root); > kfree(dwc->regset); > > > -- > Balbi No, no need anymore. Because no branch share this code now. Then remove the goto would make code a little clear. Thanks, Du, Changbin
Re: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void
Hi, changbin...@intel.com writes: > diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h > index 07fbc2d..71e3180 100644 > --- a/drivers/usb/dwc3/debug.h > +++ b/drivers/usb/dwc3/debug.h > @@ -217,11 +217,11 @@ static inline const char > *dwc3_gadget_event_type_string(u8 event) > void dwc3_trace(void (*trace)(struct va_format *), const char *fmt, ...); > > #ifdef CONFIG_DEBUG_FS > -extern int dwc3_debugfs_init(struct dwc3 *); > +extern void dwc3_debugfs_init(struct dwc3 *); > extern void dwc3_debugfs_exit(struct dwc3 *); > #else > -static inline int dwc3_debugfs_init(struct dwc3 *d) > -{ return 0; } > +static inline void dwc3_debugfs_init(struct dwc3 *d) > +{ } > static inline void dwc3_debugfs_exit(struct dwc3 *d) > { } > #endif > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c > index 9ac37fe..071b286 100644 > --- a/drivers/usb/dwc3/debugfs.c > +++ b/drivers/usb/dwc3/debugfs.c > @@ -618,69 +618,39 @@ static const struct file_operations > dwc3_link_state_fops = { > .release= single_release, > }; > > -int dwc3_debugfs_init(struct dwc3 *dwc) > +void dwc3_debugfs_init(struct dwc3 *dwc) > { > struct dentry *root; > - struct dentry *file; > - int ret; > > root = debugfs_create_dir(dev_name(dwc->dev), NULL); > - if (!root) { > - ret = -ENOMEM; > - goto err0; > - } > + if (IS_ERR_OR_NULL(root)) > + return; We can definitely keep on going, but I'd still like to know that we enabled CONFIG_DEBUG_FS but failed to create a file or a directory. Seems like this should read as follows: if (IS_ERR_OR_NULL(root)) { if (!root) dev_err(dwc->dev, "Can't create debugfs root\n"); return; } ditto to all bellow. > dwc->root = root; > > dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL); > if (!dwc->regset) { > - ret = -ENOMEM; > - goto err1; > + debugfs_remove_recursive(root); you're now duplicating debugfs_remove_recursive(root) in all braches and that's error prone. It's probably better to keep our gotos, but change them so they read as follows: if (!dwc->regset) goto err1; [...] return; /* this is our successful exit point */ err1: debugfs_remove_recursive(root); kfree(dwc->regset); -- balbi signature.asc Description: PGP signature
Re: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void
Hi, changbin...@intel.com writes: > diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h > index 07fbc2d..71e3180 100644 > --- a/drivers/usb/dwc3/debug.h > +++ b/drivers/usb/dwc3/debug.h > @@ -217,11 +217,11 @@ static inline const char > *dwc3_gadget_event_type_string(u8 event) > void dwc3_trace(void (*trace)(struct va_format *), const char *fmt, ...); > > #ifdef CONFIG_DEBUG_FS > -extern int dwc3_debugfs_init(struct dwc3 *); > +extern void dwc3_debugfs_init(struct dwc3 *); > extern void dwc3_debugfs_exit(struct dwc3 *); > #else > -static inline int dwc3_debugfs_init(struct dwc3 *d) > -{ return 0; } > +static inline void dwc3_debugfs_init(struct dwc3 *d) > +{ } > static inline void dwc3_debugfs_exit(struct dwc3 *d) > { } > #endif > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c > index 9ac37fe..071b286 100644 > --- a/drivers/usb/dwc3/debugfs.c > +++ b/drivers/usb/dwc3/debugfs.c > @@ -618,69 +618,39 @@ static const struct file_operations > dwc3_link_state_fops = { > .release= single_release, > }; > > -int dwc3_debugfs_init(struct dwc3 *dwc) > +void dwc3_debugfs_init(struct dwc3 *dwc) > { > struct dentry *root; > - struct dentry *file; > - int ret; > > root = debugfs_create_dir(dev_name(dwc->dev), NULL); > - if (!root) { > - ret = -ENOMEM; > - goto err0; > - } > + if (IS_ERR_OR_NULL(root)) > + return; We can definitely keep on going, but I'd still like to know that we enabled CONFIG_DEBUG_FS but failed to create a file or a directory. Seems like this should read as follows: if (IS_ERR_OR_NULL(root)) { if (!root) dev_err(dwc->dev, "Can't create debugfs root\n"); return; } ditto to all bellow. > dwc->root = root; > > dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL); > if (!dwc->regset) { > - ret = -ENOMEM; > - goto err1; > + debugfs_remove_recursive(root); you're now duplicating debugfs_remove_recursive(root) in all braches and that's error prone. It's probably better to keep our gotos, but change them so they read as follows: if (!dwc->regset) goto err1; [...] return; /* this is our successful exit point */ err1: debugfs_remove_recursive(root); kfree(dwc->regset); -- balbi signature.asc Description: PGP signature