Hello, Grant. Grant Grundler wrote: >> -/* >> - * Dynamic device IDs are disabled for !CONFIG_HOTPLUG >> - */ > > The comment was removed here. Ok. Yeap, because dynids as a whole are no longer dependent on CONFIG_HOTPLUG. >> + spin_lock(&drv->dynids.lock); >> + list_add_tail(&dynid->node, &drv->dynids.list); >> + spin_unlock(&drv->dynids.lock); >> + >> + get_driver(&drv->driver); > > Return value ignored caught my attention... But I can't say if that's wrong. > > I'm not finding any documentation on get_driver() in Documentation/ . > Looking at drivers/base/driver.c:get_driver() code, I'm confused why this > function bothers returning struct device_driver *. My expectation is > the input parameter "drv" is the same as "priv->driver" that gets returned. > No? Yes, it is. > In any case, if the Docbook comments for get_driver() could explain > whatever subtle difference there is, that would be helpful. It's one of the gripes I've had with the driver core interface. Returning the object which had its reference bumped up is nice to do do_something(get_driver(driver)); but it should have been made clear that the return value always equals the input parameter. IIRC, in the early days of the current driver model and kref, there were intentions to make kref somehow more capable and handle the last reference problem from within it so that if (get_driver(driver)) test is actually meaningful but that turned out to be not too feasible, so we ended up with unnecessary if () around driver model get functions. I try to remove them whenever an affected piece of code is being modified but there still are plenty left. I'll update comment on driver core get functions. >> +/* >> + * Dynamic device ID manipulation via sysfs is disabled for !CONFIG_HOTPLUG >> + */ > > But same comment added back here. Is this comment correct? Yes, dynid in general is enabled regardless but sysfs access is available iff CONFIG_HOTPLUG. > If comments are the only thing that change, please add: > Reviewed-by: Grant Grundler <grundler@xxxxxxxxxxxxxxxx> > > This patch seems pretty straight forward. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
