Re: [PATCH] pci: separate out pci_add_dynid()

[Tejun Heo]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/