On 11/26/20 4:12 PM, Benjamin Block wrote:
On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote:
在 2020/11/26 17:42, Benjamin Block 写道:
On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote:
On Thu, 26 Nov 2020 09:27:41 +0800
Qinglang Miao <miaoqinglang@xxxxxxxxxx> wrote:
在 2020/11/26 1:06, Benjamin Block 写道:
On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote:
....
Let's go by example. If we assume the reference count of `unit->dev` is
R, and the function starts with R = 1 (otherwise the deivce would've
been freed already), we get:
int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun)
{
struct zfcp_unit *unit;
struct scsi_device *sdev;
write_lock_irq(&port->unit_list_lock);
// unit->dev (R = 1)
unit = _zfcp_unit_find(port, fcp_lun);
// get_device(&unit->dev)
// unit->dev (R = 2)
if (unit)
list_del(&unit->list);
write_unlock_irq(&port->unit_list_lock);
if (!unit)
return -EINVAL;
sdev = zfcp_unit_sdev(unit);
if (sdev) {
scsi_remove_device(sdev);
scsi_device_put(sdev);
}
// unit->dev (R = 2)
put_device(&unit->dev);
// unit->dev (R = 1)
device_unregister(&unit->dev);
// unit->dev (R = 0)
return 0;
}
If we now apply this patch, we'd end up with R = 1 after
`device_unregister()`, and the device would not be properly removed.
If you still think that's wrong, then you'll need to better explain why.
Hi Banjamin and Cornelia,
Your replies make me reliaze that I've been holding a mistake understanding
of put_device() as well as reference count.
Thanks for you two's patient explanation !!
BTW, should I send a v2 on these two patches to move the position of
put_device()?
Feel free to do so.
I think having the `put_device()` call after `device_unregister()` in
both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more
natural, because it ought to be the last time we touch the object in
both functions.
If you move put_device(), you could add a comment like we did here to explain
which (hidden) get_device is undone:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=ef4021fe5fd77ced0323cede27979d80a56211ca
("scsi: zfcp: fix to prevent port_remove with pure auto scan LUNs (only sdevs)")
So in this patch it could be:
put_device(&unit->dev); /* undo _zfcp_unit_find() */
And in the other patch it could be:
put_device(&port->dev); /* undo zfcp_get_port_by_wwpn() */
Then it would be clearer next time somebody looks at the code.
Especially for the other patch on zfcp_sysfs_port_remove_store() moving the
put_device(&port->dev) to at least *after* the call of
zfcp_erp_port_shutdown(port, 0, "syprs_1") would make the code cleaner to me.
Along the idead of passing the port to zfcp_erp_port_shutdown with the
reference we got from zfcp_get_port_by_wwpn(). That said, the current code is
of course still correct as we currently have the port ref of the earlier
device_register so passing the port to zfcp_erp_port_shutdown() is safe.
If we wanted to make the gets and puts nicely nested, then we could move the
puts to just before the device_unregister, but that's bike shedding:
device_register() --+
get_device() --+ |
put_device() --+ |
device_unregister() --+
Benjamin's suggested move location works for me, too. After all, the kdoc of
device_unregister explicitly mentions the possibility that other refs might
continue to exist after device_unregister was called:
device_register() --+
get_device() ---------|--+
device_unregister() --+ |
put_device() ------------+
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z Development
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
