[PATCH v2 0/5] dax-device: Some cleanups

[PATCH v2 0/5] dax-device: Some cleanups

[Uwe Kleine-König]

Hello,

I didn't get any feedback for the (implicit) v1 of this series that
started with Message-Id: 20210127230124.109522-1-uwe@kleine-koenig.org,
but I identified a few improvements myself:

 - Use "dax-device" consistently as a prefix
 - Instead of requiring a .remove callback, make it explicitly
   optional. (Drop checking for .remove from former patch 1, introduce
   new patch "Properly handle drivers without remove callback")
 - The new patch about remove being optional allows to simplify one of
   the two dax drivers which is implemented in patch 4
 - Patch 5 got a bit smaller because we now have one driver less with a
   remove callback.
 - Added Andrew to To: as he merged dax drivers in the past.

Andrew: Assuming you consider these patches useful, would you please
care for merging them?

Best regards
Uwe

Uwe Kleine-König (5):
  dax-device: Prevent registering drivers without probe callback
  dax-device: Properly handle drivers without remove callback
  dax-device: Fix error path in dax_driver_register
  dax-device: Drop an empty .remove callback
  dax-device: Make remove callback return void

 drivers/dax/bus.c    | 22 ++++++++++++++++++++--
 drivers/dax/bus.h    |  2 +-
 drivers/dax/device.c |  8 +-------
 drivers/dax/kmem.c   |  7 ++-----
 4 files changed, 24 insertions(+), 15 deletions(-)


base-commit: 5c8fe583cce542aa0b84adc939ce85293de36e5e
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v2 1/5] dax-device: Prevent registering drivers without probe callback

[Uwe Kleine-König]

The bus probe function dax_bus_probe() calls the probe callback without
checking it to be non-NULL. Prevent a NULL pointer exception if a driver
without a probe function is registered by refusing to register this
driver.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/dax/bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 737b207c9e30..72fc4b9b9ae6 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -1392,6 +1392,13 @@ int __dax_driver_register(struct dax_device_driver *dax_drv,
 	struct device_driver *drv = &dax_drv->drv;
 	int rc = 0;
 
+	/*
+	 * dax_bus_probe() calls dax_drv->probe() unconditionally.
+	 * So better be safe than sorry and ensure it is provided.
+	 */
+	if (!dax_drv->probe)
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&dax_drv->ids);
 	drv->owner = module;
 	drv->name = mod_name;
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v2 2/5] dax-device: Properly handle drivers without remove callback

[Uwe Kleine-König]

If all resources are allocated in .probe() using devm_ functions it
might make sense to not provide a .remove() callback. Then the right
thing is to just return success.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/dax/bus.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 72fc4b9b9ae6..2c9e3f6f615f 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -178,8 +178,12 @@ static int dax_bus_remove(struct device *dev)
 {
 	struct dax_device_driver *dax_drv = to_dax_drv(dev->driver);
 	struct dev_dax *dev_dax = to_dev_dax(dev);
+	int ret = 0;
 
-	return dax_drv->remove(dev_dax);
+	if (dax_drv->remove)
+		ret = dax_drv->remove(dev_dax);
+
+	return ret;
 }
 
 static struct bus_type dax_bus_type = {
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v2 3/5] dax-device: Fix error path in dax_driver_register

[Uwe Kleine-König]

The static variable match_always_count is supposed to track if there is
a driver registered that has .match_always set. If driver_register()
fails, the previous increment must be undone.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/dax/bus.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 2c9e3f6f615f..bc425f1c52bd 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -1420,7 +1420,15 @@ int __dax_driver_register(struct dax_device_driver *dax_drv,
 	mutex_unlock(&dax_bus_lock);
 	if (rc)
 		return rc;
-	return driver_register(drv);
+
+	rc = driver_register(drv);
+	if (rc && dax_drv->match_always) {
+		mutex_lock(&dax_bus_lock);
+		match_always_count -= dax_drv->match_always;
+		mutex_unlock(&dax_bus_lock);
+	}
+
+	return rc;
 }
 EXPORT_SYMBOL_GPL(__dax_driver_register);
 
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v2 4/5] dax-device: Drop an empty .remove callback

[Uwe Kleine-König]

The dax core properly handles a dax driver not having a remove callback.
So drop it without changing the effective behaviour.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/dax/device.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 5da2980bb16b..db92573c94e8 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -452,15 +452,9 @@ int dev_dax_probe(struct dev_dax *dev_dax)
 }
 EXPORT_SYMBOL_GPL(dev_dax_probe);
 
-static int dev_dax_remove(struct dev_dax *dev_dax)
-{
-	/* all probe actions are unwound by devm */
-	return 0;
-}
-
 static struct dax_device_driver device_dax_driver = {
 	.probe = dev_dax_probe,
-	.remove = dev_dax_remove,
+	/* all probe actions are unwound by devm, so .remove isn't necessary */
 	.match_always = 1,
 };
 
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v2 5/5] dax-device: Make remove callback return void

[Uwe Kleine-König]

The driver core ignores the return value of struct bus_type::remove()
because there is only little that can be done. To simplify the quest to
make this function return void, let struct dax_device_driver::remove()
return void, too. All users already unconditionally return 0, this commit
makes it obvious that returning an error code isn't intended.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/dax/bus.c  | 5 ++---
 drivers/dax/bus.h  | 2 +-
 drivers/dax/kmem.c | 7 ++-----
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index bc425f1c52bd..0a939e28d048 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -178,12 +178,11 @@ static int dax_bus_remove(struct device *dev)
 {
 	struct dax_device_driver *dax_drv = to_dax_drv(dev->driver);
 	struct dev_dax *dev_dax = to_dev_dax(dev);
-	int ret = 0;
 
 	if (dax_drv->remove)
-		ret = dax_drv->remove(dev_dax);
+		dax_drv->remove(dev_dax);
 
-	return ret;
+	return 0;
 }
 
 static struct bus_type dax_bus_type = {
diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h
index 72b92f95509f..1e946ad7780a 100644
--- a/drivers/dax/bus.h
+++ b/drivers/dax/bus.h
@@ -39,7 +39,7 @@ struct dax_device_driver {
 	struct list_head ids;
 	int match_always;
 	int (*probe)(struct dev_dax *dev);
-	int (*remove)(struct dev_dax *dev);
+	void (*remove)(struct dev_dax *dev);
 };
 
 int __dax_driver_register(struct dax_device_driver *dax_drv,
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
index 403ec42472d1..ac231cc36359 100644
--- a/drivers/dax/kmem.c
+++ b/drivers/dax/kmem.c
@@ -136,7 +136,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
 }
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
-static int dev_dax_kmem_remove(struct dev_dax *dev_dax)
+static void dev_dax_kmem_remove(struct dev_dax *dev_dax)
 {
 	int i, success = 0;
 	struct device *dev = &dev_dax->dev;
@@ -176,11 +176,9 @@ static int dev_dax_kmem_remove(struct dev_dax *dev_dax)
 		kfree(data);
 		dev_set_drvdata(dev, NULL);
 	}
-
-	return 0;
 }
 #else
-static int dev_dax_kmem_remove(struct dev_dax *dev_dax)
+static void dev_dax_kmem_remove(struct dev_dax *dev_dax)
 {
 	/*
 	 * Without hotremove purposely leak the request_mem_region() for the
@@ -190,7 +188,6 @@ static int dev_dax_kmem_remove(struct dev_dax *dev_dax)
 	 * request_mem_region().
 	 */
 	any_hotremove_failed = true;
-	return 0;
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v2 0/5] dax-device: Some cleanups

[Dan Williams]

On Fri, Feb 5, 2021 at 2:29 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> Hello,
>
> I didn't get any feedback for the (implicit) v1 of this series that
> started with Message-Id: 20210127230124.109522-1-uwe@kleine-koenig.org,
> but I identified a few improvements myself:
>
>  - Use "dax-device" consistently as a prefix
>  - Instead of requiring a .remove callback, make it explicitly
>    optional. (Drop checking for .remove from former patch 1, introduce
>    new patch "Properly handle drivers without remove callback")
>  - The new patch about remove being optional allows to simplify one of
>    the two dax drivers which is implemented in patch 4
>  - Patch 5 got a bit smaller because we now have one driver less with a
>    remove callback.
>  - Added Andrew to To: as he merged dax drivers in the past.
>
> Andrew: Assuming you consider these patches useful, would you please
> care for merging them?

I've routed device-dax patches through Andrew when they had core-mm
entanglements, but a pure device-dax series like this I can take
through my tree.

One small comment on patch5, otherwise looks good.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v2 0/5] dax-device: Some cleanups

[Dan Williams]

On Tue, Feb 16, 2021 at 7:48 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Fri, Feb 5, 2021 at 2:29 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> >
> > Hello,
> >
> > I didn't get any feedback for the (implicit) v1 of this series that
> > started with Message-Id: 20210127230124.109522-1-uwe@kleine-koenig.org,
> > but I identified a few improvements myself:
> >
> >  - Use "dax-device" consistently as a prefix
> >  - Instead of requiring a .remove callback, make it explicitly
> >    optional. (Drop checking for .remove from former patch 1, introduce
> >    new patch "Properly handle drivers without remove callback")
> >  - The new patch about remove being optional allows to simplify one of
> >    the two dax drivers which is implemented in patch 4
> >  - Patch 5 got a bit smaller because we now have one driver less with a
> >    remove callback.
> >  - Added Andrew to To: as he merged dax drivers in the past.
> >
> > Andrew: Assuming you consider these patches useful, would you please
> > care for merging them?
>
> I've routed device-dax patches through Andrew when they had core-mm
> entanglements, but a pure device-dax series like this I can take
> through my tree.
>
> One small comment on patch5, otherwise looks good.

I take it back, patch5 looks good. I was going to ask about the return
value removal for dax_bus_remove(), but that would need struct
bus_type to change prototypes.

All merged to the nvdimm tree.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v2 0/5] dax-device: Some cleanups

[Uwe Kleine-König]

[-- Attachment #1.1.1: Type: text/plain, Size: 619 bytes --]

Hello Dan,

On 2/17/21 4:55 AM, Dan Williams wrote:
>> One small comment on patch5, otherwise looks good.
> 
> I take it back, patch5 looks good. I was going to ask about the return
> value removal for dax_bus_remove(), but that would need struct
> bus_type to change prototypes.

Changing struct bus_type::remove to return void is the eventual plan. To 
make this a pretty and easily reviewable patch I currently go through 
all buses and make sure that for the prototype change I only have to do 
one s/int/void/ and drop a "return 0" per bus.

> All merged to the nvdimm tree.

Great, thanks
Uwe


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org