10:28 p.m.
Hello,
I didn't get any feedback for the (implicit) v1 of this series that
started with Message-Id: 20210127230124.109522-1-uwe(a)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
10:28 p.m.
New subject: [PATCH v2 1/5] dax-device: Prevent registering drivers without probe callback
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(a)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
10:28 p.m.
New subject: [PATCH v2 2/5] dax-device: Properly handle drivers without remove callback
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(a)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
10:28 p.m.
New subject: [PATCH v2 3/5] dax-device: Fix error path in dax_driver_register
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(a)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
10:28 p.m.
New subject: [PATCH v2 4/5] dax-device: Drop an empty .remove callback
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(a)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
10:28 p.m.
New subject: [PATCH v2 5/5] dax-device: Make remove callback return void
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(a)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
3:48 a.m.
On Fri, Feb 5, 2021 at 2:29 PM Uwe Kleine-König <uwe(a)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(a)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.
3:55 a.m.
On Tue, Feb 16, 2021 at 7:48 PM Dan Williams <dan.j.williams(a)intel.com> wrote:
On Fri, Feb 5, 2021 at 2:29 PM Uwe Kleine-König <uwe(a)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(a)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.
6:39 a.m.
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
545
days inactive
557
days old
8 comments
2 participants
participants (2)
-
Dan Williams -
Uwe Kleine-König