Hi,
"Du, Changbin" <changbin.du@xxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
