| Message ID | 20210805183100.49071-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1,1/1] wwan: core: Avoid returning error pointer from wwan_create_dev() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, 5 Aug 2021 at 20:38, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > wwan_create_dev() is expected to return either valid pointer or NULL, > In some cases it might return the error pointer. Prevent this by converting > it to NULL after wwan_dev_get_by_parent(). wwan_create_dev is called both from wwan_register_ops() and wwan_create_port(), one using IS_ERR and the other using NULL testing, they should be aligned as well. Regards, Loic
On Thu, Aug 05, 2021 at 09:53:57PM +0200, Loic Poulain wrote: > On Thu, 5 Aug 2021 at 20:38, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > wwan_create_dev() is expected to return either valid pointer or NULL, > > In some cases it might return the error pointer. Prevent this by converting > > it to NULL after wwan_dev_get_by_parent(). > > wwan_create_dev is called both from wwan_register_ops() and > wwan_create_port(), one using IS_ERR and the other using NULL testing, > they should be aligned as well. Ah, good catch! I just sent v2, but eventually I have decided to switch to error pointer since it seems the most used pattern in the code.
Hello Andy, On Fri, Aug 6, 2021 at 12:08 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, Aug 05, 2021 at 09:53:57PM +0200, Loic Poulain wrote: >> On Thu, 5 Aug 2021 at 20:38, Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >>> >>> wwan_create_dev() is expected to return either valid pointer or NULL, >>> In some cases it might return the error pointer. Prevent this by converting >>> it to NULL after wwan_dev_get_by_parent(). >> >> wwan_create_dev is called both from wwan_register_ops() and >> wwan_create_port(), one using IS_ERR and the other using NULL testing, >> they should be aligned as well. > > Ah, good catch! > > I just sent v2, but eventually I have decided to switch to error pointer since > it seems the most used pattern in the code. I agree that returning the error pointer is a good solution here. Thank you for the fix.
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c index 674a81d79db3..35e10a98e774 100644 --- a/drivers/net/wwan/wwan_core.c +++ b/drivers/net/wwan/wwan_core.c @@ -160,7 +160,9 @@ static struct wwan_device *wwan_create_dev(struct device *parent) /* If wwandev already exists, return it */ wwandev = wwan_dev_get_by_parent(parent); - if (!IS_ERR(wwandev)) + if (IS_ERR(wwandev)) + wwandev = NULL; + else goto done_unlock; id = ida_alloc(&wwan_dev_ids, GFP_KERNEL);
wwan_create_dev() is expected to return either valid pointer or NULL, In some cases it might return the error pointer. Prevent this by converting it to NULL after wwan_dev_get_by_parent(). Fixes: 9a44c1cc6388 ("net: Add a WWAN subsystem") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/net/wwan/wwan_core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)