Messages in this thread | | | Date | Tue, 22 Jul 2014 15:01:20 -0400 | From | Tejun Heo <> | Subject | Re: [PATCH 1/2] resource: Add device-managed request/release_resource() |
| |
Hello,
On Tue, Jul 22, 2014 at 12:50:02PM -0600, Bjorn Helgaas wrote: > [+cc Tejun, LKML] > > On Fri, Jul 11, 2014 at 09:08:24AM +0200, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Provide device-managed implementations of the request_resource() and > > release_resource() functions. Upon failure to request a resource, the > > new devm_request_resource() function will output an error message for > > consistent error reporting. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > This seems OK to me, but I don't consider myself a devres maintainer. I > added Tejun and LKML for any comment. Minor nit below.
If there are gonna be users of the interface, sure.
> > +int devm_request_resource(struct device *dev, struct resource *root, > > + struct resource *new) > > +{ > > + struct resource *conflict, **ptr; > > + > > + ptr = devres_alloc(devm_resource_release, sizeof(*ptr), GFP_KERNEL); > > + if (!ptr) > > + return -ENOMEM; > > + > > + *ptr = new; > > + > > + conflict = request_resource_conflict(root, new); > > + if (!conflict) { > > + devres_add(dev, ptr); > > + return 0; > > + } > > + > > + dev_err(dev, "resource collision: %pR conflicts with %s %pR\n", new, > > + conflict->name, conflict); > > + devres_free(ptr); > > + return -EBUSY; > > Personally I would write this as: > > conflict = request_resource_conflict(...); > if (conflict) { > dev_err(...); > devres_free(...); > return -EBUSY; > } > > devres_add(...); > return 0; > > so the straight-line path is the normal, non-error path and errors are > detected and dealt with in the "if" bodies. Right now the "if" bodies > are a mix of error handling and normal path. But that's just my personal > preference.
Agreed.
> > +static int devm_resource_match(struct device *dev, void *res, void *data) > > +{ > > + struct resource **ptr = res; > > + > > + if (WARN_ON(!ptr || !*ptr)) > > + return 0;
How would !ptr or !*ptr possibly happen? Wouldn't that be a bug already?
Thanks.
-- tejun
| |