| Message ID | 1549642921-11196-1-git-send-email-parav@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] devlink: Add WARN_ON to catch errors of not cleaning devlink objects | expand |
On 2/8/19 8:22 AM, Parav Pandit wrote: > Add WARN_ON to make sure that all sub objects of a devlink device are > cleanedup before freeing the devlink device. > This helps to catch any driver bugs. > > Signed-off-by: Parav Pandit <parav@mellanox.com> > Acked-by: Jiri Pirko <jiri@mellanox.com> > --- > net/core/devlink.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/core/devlink.c b/net/core/devlink.c > index cd0d393..5e2ef5a 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -4229,6 +4229,13 @@ void devlink_unregister(struct devlink *devlink) > */ > void devlink_free(struct devlink *devlink) > { > + WARN_ON(!list_empty(&devlink->port_list)); > + WARN_ON(!list_empty(&devlink->sb_list)); > + WARN_ON(!list_empty(&devlink->dpipe_table_list)); > + WARN_ON(!list_empty(&devlink->resource_list)); > + WARN_ON(!list_empty(&devlink->param_list)); > + WARN_ON(!list_empty(&devlink->region_list)); > + > kfree(devlink); > } > EXPORT_SYMBOL_GPL(devlink_free); > reporter_list was just added which brings up the maintenance question: If you are going to do this you might want a comment in include/net/devlink.h to remind folks to update this function as relevant.
> -----Original Message----- > From: David Ahern <dsahern@gmail.com> > Sent: Friday, February 8, 2019 11:30 AM > To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org; > davem@davemloft.net > Subject: Re: [PATCH net-next] devlink: Add WARN_ON to catch errors of not > cleaning devlink objects > > On 2/8/19 8:22 AM, Parav Pandit wrote: > > Add WARN_ON to make sure that all sub objects of a devlink device are > > cleanedup before freeing the devlink device. > > This helps to catch any driver bugs. > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > Acked-by: Jiri Pirko <jiri@mellanox.com> > > --- > > net/core/devlink.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/net/core/devlink.c b/net/core/devlink.c index > > cd0d393..5e2ef5a 100644 > > --- a/net/core/devlink.c > > +++ b/net/core/devlink.c > > @@ -4229,6 +4229,13 @@ void devlink_unregister(struct devlink *devlink) > > */ > > void devlink_free(struct devlink *devlink) { > > + WARN_ON(!list_empty(&devlink->port_list)); > > + WARN_ON(!list_empty(&devlink->sb_list)); > > + WARN_ON(!list_empty(&devlink->dpipe_table_list)); > > + WARN_ON(!list_empty(&devlink->resource_list)); > > + WARN_ON(!list_empty(&devlink->param_list)); > > + WARN_ON(!list_empty(&devlink->region_list)); > > + > > kfree(devlink); > > } > > EXPORT_SYMBOL_GPL(devlink_free); > > > > reporter_list was just added which brings up the maintenance question: > If you are going to do this you might want a comment in > include/net/devlink.h to remind folks to update this function as relevant. I see. Make sense. Adding reporter_list and updating devlink.h, too for comment in v1.
> -----Original Message----- > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On > Behalf Of Parav Pandit > Sent: Friday, February 8, 2019 12:01 PM > To: David Ahern <dsahern@gmail.com>; netdev@vger.kernel.org; > davem@davemloft.net > Subject: RE: [PATCH net-next] devlink: Add WARN_ON to catch errors of not > cleaning devlink objects > > > > > -----Original Message----- > > From: David Ahern <dsahern@gmail.com> > > Sent: Friday, February 8, 2019 11:30 AM > > To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org; > > davem@davemloft.net > > Subject: Re: [PATCH net-next] devlink: Add WARN_ON to catch errors of > > not cleaning devlink objects > > > > On 2/8/19 8:22 AM, Parav Pandit wrote: > > > Add WARN_ON to make sure that all sub objects of a devlink device > > > are cleanedup before freeing the devlink device. > > > This helps to catch any driver bugs. > > > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > > Acked-by: Jiri Pirko <jiri@mellanox.com> > > > --- > > > net/core/devlink.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/net/core/devlink.c b/net/core/devlink.c index > > > cd0d393..5e2ef5a 100644 > > > --- a/net/core/devlink.c > > > +++ b/net/core/devlink.c > > > @@ -4229,6 +4229,13 @@ void devlink_unregister(struct devlink > *devlink) > > > */ > > > void devlink_free(struct devlink *devlink) { > > > + WARN_ON(!list_empty(&devlink->port_list)); > > > + WARN_ON(!list_empty(&devlink->sb_list)); > > > + WARN_ON(!list_empty(&devlink->dpipe_table_list)); > > > + WARN_ON(!list_empty(&devlink->resource_list)); > > > + WARN_ON(!list_empty(&devlink->param_list)); > > > + WARN_ON(!list_empty(&devlink->region_list)); > > > + > > > kfree(devlink); > > > } > > > EXPORT_SYMBOL_GPL(devlink_free); > > > > > > > reporter_list was just added which brings up the maintenance question: > > If you are going to do this you might want a comment in > > include/net/devlink.h to remind folks to update this function as relevant. > I see. Make sense. Adding reporter_list and updating devlink.h, too for > comment in v1. I think its little too much to add such a generic comment in devlink.h for lists. My tree was bit old and missed out the reporter_list entry because we first test on internal trees. I updated my tree to avoid such problem in future.
diff --git a/net/core/devlink.c b/net/core/devlink.c index cd0d393..5e2ef5a 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -4229,6 +4229,13 @@ void devlink_unregister(struct devlink *devlink) */ void devlink_free(struct devlink *devlink) { + WARN_ON(!list_empty(&devlink->port_list)); + WARN_ON(!list_empty(&devlink->sb_list)); + WARN_ON(!list_empty(&devlink->dpipe_table_list)); + WARN_ON(!list_empty(&devlink->resource_list)); + WARN_ON(!list_empty(&devlink->param_list)); + WARN_ON(!list_empty(&devlink->region_list)); + kfree(devlink); } EXPORT_SYMBOL_GPL(devlink_free);