Hi, "Du, Changbin" <[email protected]> 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
