Hello,
I didn't get any feedback for the (implicit) v1 of this series that
started with Message-Id: [email protected],
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Feb 5, 2021 at 2:29 PM Uwe Kleine-König <[email protected]> wrote:
>
> Hello,
>
> I didn't get any feedback for the (implicit) v1 of this series that
> started with Message-Id: [email protected],
> 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.
On Tue, Feb 16, 2021 at 7:48 PM Dan Williams <[email protected]> wrote:
>
> On Fri, Feb 5, 2021 at 2:29 PM Uwe Kleine-König <[email protected]> wrote:
> >
> > Hello,
> >
> > I didn't get any feedback for the (implicit) v1 of this series that
> > started with Message-Id: [email protected],
> > 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.
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