[PATCH 1/1] nvme: fix CMB sysfs file removal in reset path

[PATCH 1/1] nvme: fix CMB sysfs file removal in reset path

[Max Gurtovoy]

Currently we create the sysfs entry even if we fail mapping
it. In that case, the unmapping will not remove the sysfs created
file.

Fixes: f63572dff ("nvme: unmap CMB and remove sysfs file in reset path")

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/pci.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8569ee7..cbbfc68 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1558,12 +1558,13 @@ static inline void nvme_release_cmb(struct nvme_dev *dev)
 	if (dev->cmb) {
 		iounmap(dev->cmb);
 		dev->cmb = NULL;
-		if (dev->cmbsz) {
-			sysfs_remove_file_from_group(&dev->ctrl.device->kobj,
-						     &dev_attr_cmb.attr, NULL);
-			dev->cmbsz = 0;
-		}
 	}
+	if (NVME_CMB_SZ(dev->cmbsz)) {
+		sysfs_remove_file_from_group(&dev->ctrl.device->kobj,
+					     &dev_attr_cmb.attr, NULL);
+		dev->cmbsz = 0;
+	}
+
 }
 
 static int nvme_set_host_mem(struct nvme_dev *dev, u32 bits)
@@ -1962,7 +1963,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2, 0)) {
 		dev->cmb = nvme_map_cmb(dev);
 
-		if (dev->cmbsz) {
+		if (NVME_CMB_SZ(dev->cmbsz)) {
 			if (sysfs_add_file_to_group(&dev->ctrl.device->kobj,
 						    &dev_attr_cmb.attr, NULL))
 				dev_warn(dev->ctrl.device,
-- 
1.7.1

[PATCH 1/1] nvme: fix CMB sysfs file removal in reset path

[Christoph Hellwig]

On Tue, Jul 25, 2017@04:58:36PM +0300, Max Gurtovoy wrote:
> Currently we create the sysfs entry even if we fail mapping
> it. In that case, the unmapping will not remove the sysfs created
> file.
> 
> Fixes: f63572dff ("nvme: unmap CMB and remove sysfs file in reset path")

This looks correct, but I wonder if the better plan would be to only
add the sysfs file if we have a working CMB?

Also the comment about the auto-removal above the call to
sysfs_add_file_to_group for dev_attr_cmb looks wrong to me, that
might be worth fixing up as well.

[PATCH 1/1] nvme: fix CMB sysfs file removal in reset path

[Max Gurtovoy]

On 7/25/2017 6:56 PM, Christoph Hellwig wrote:
> On Tue, Jul 25, 2017@04:58:36PM +0300, Max Gurtovoy wrote:
>> Currently we create the sysfs entry even if we fail mapping
>> it. In that case, the unmapping will not remove the sysfs created
>> file.
>>
>> Fixes: f63572dff ("nvme: unmap CMB and remove sysfs file in reset path")
>
> This looks correct, but I wonder if the better plan would be to only
> add the sysfs file if we have a working CMB?

Yes I thought about it too, but in case we don't want to "use_cmb_sqes"
and still want to see the device CMB capabilities, using this sysfs we 
could.
Why to do it ? for using this iomem for p2pmem for example.
Since, p2pmem is not yet upstream we can go with working CMB approach.

>
> Also the comment about the auto-removal above the call to
> sysfs_add_file_to_group for dev_attr_cmb looks wrong to me, that
> might be worth fixing up as well.

Sure, I'll update it in V2.