Re: [PATCH] vio: make remove callback return void
On Wed, Jan 27, 2021 at 10:50:10PM +0100, Uwe Kleine-König wrote: > 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 vio_driver::remove() return > void, too. All users already unconditionally return 0, this commit makes > it obvious that returning an error code is a bad idea and makes it > obvious for future driver authors that returning an error code isn't > intended. > > Note there are two nominally different implementations for a vio bus: > one in arch/sparc/kernel/vio.c and the other in > arch/powerpc/platforms/pseries/vio.c. I didn't care to check which > driver is using which of these busses (or if even some of them can be > used with both) and simply adapt all drivers and the two bus codes in > one go. > > Note that for the powerpc implementation there is a semantical change: > Before this patch for a device that was bound to a driver without a > remove callback vio_cmo_bus_remove(viodev) wasn't called. As the device > core still considers the device unbound after vio_bus_remove() returns > calling this unconditionally is the consistent behaviour which is > implemented here. > > Signed-off-by: Uwe Kleine-König > --- > Hello, > > note that this change depends on > https://lore.kernel.org/r/20210121062005.53271-1-...@linux.ibm.com which > removes > an (ignored) return -EBUSY in drivers/net/ethernet/ibm/ibmvnic.c. > I don't know when/if this latter patch will be applied, so it might take > some time until my patch can go in. Acked-by: Greg Kroah-Hartman
Re: [PATCH] vio: make remove callback return void
On Wed, Jan 27, 2021 at 6:41 PM Uwe Kleine-König wrote: > > 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 vio_driver::remove() return > void, too. All users already unconditionally return 0, this commit makes > it obvious that returning an error code is a bad idea and makes it > obvious for future driver authors that returning an error code isn't > intended. > > Note there are two nominally different implementations for a vio bus: > one in arch/sparc/kernel/vio.c and the other in > arch/powerpc/platforms/pseries/vio.c. I didn't care to check which > driver is using which of these busses (or if even some of them can be > used with both) and simply adapt all drivers and the two bus codes in > one go. > > Note that for the powerpc implementation there is a semantical change: > Before this patch for a device that was bound to a driver without a > remove callback vio_cmo_bus_remove(viodev) wasn't called. As the device > core still considers the device unbound after vio_bus_remove() returns > calling this unconditionally is the consistent behaviour which is > implemented here. > > Signed-off-by: Uwe Kleine-König Acked-by: Lijun Pan
Re: [PATCH] vio: make remove callback return void
On 1/27/21 1:50 PM, Uwe Kleine-König wrote:
> 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 vio_driver::remove() return
> void, too. All users already unconditionally return 0, this commit makes
> it obvious that returning an error code is a bad idea and makes it
> obvious for future driver authors that returning an error code isn't
> intended.
>
> Note there are two nominally different implementations for a vio bus:
> one in arch/sparc/kernel/vio.c and the other in
> arch/powerpc/platforms/pseries/vio.c. I didn't care to check which
> driver is using which of these busses (or if even some of them can be
> used with both) and simply adapt all drivers and the two bus codes in
> one go.
>
> Note that for the powerpc implementation there is a semantical change:
> Before this patch for a device that was bound to a driver without a
> remove callback vio_cmo_bus_remove(viodev) wasn't called. As the device
> core still considers the device unbound after vio_bus_remove() returns
> calling this unconditionally is the consistent behaviour which is
> implemented here.
>
> Signed-off-by: Uwe Kleine-König
Reviewed-by: Tyrel Datwyler
> ---
> Hello,
>
> note that this change depends on
> https://lore.kernel.org/r/20210121062005.53271-1-...@linux.ibm.com which
> removes
> an (ignored) return -EBUSY in drivers/net/ethernet/ibm/ibmvnic.c.
> I don't know when/if this latter patch will be applied, so it might take
> some time until my patch can go in.
>
> Best regards
> Uwe
>
> arch/powerpc/include/asm/vio.h | 2 +-
> arch/powerpc/platforms/pseries/vio.c | 7 +++
> arch/sparc/include/asm/vio.h | 2 +-
> arch/sparc/kernel/ds.c | 6 --
> arch/sparc/kernel/vio.c | 4 ++--
> drivers/block/sunvdc.c | 3 +--
> drivers/char/hw_random/pseries-rng.c | 3 +--
> drivers/char/tpm/tpm_ibmvtpm.c | 4 +---
> drivers/crypto/nx/nx-842-pseries.c | 4 +---
> drivers/crypto/nx/nx.c | 4 +---
> drivers/misc/ibmvmc.c| 4 +---
> drivers/net/ethernet/ibm/ibmveth.c | 4 +---
> drivers/net/ethernet/ibm/ibmvnic.c | 4 +---
> drivers/net/ethernet/sun/ldmvsw.c| 4 +---
> drivers/net/ethernet/sun/sunvnet.c | 3 +--
> drivers/scsi/ibmvscsi/ibmvfc.c | 3 +--
> drivers/scsi/ibmvscsi/ibmvscsi.c | 4 +---
> drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 4 +---
> drivers/tty/hvc/hvcs.c | 3 +--
> drivers/tty/vcc.c| 4 +---
> 20 files changed, 22 insertions(+), 54 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/vio.h b/arch/powerpc/include/asm/vio.h
> index 0cf52746531b..721c0d6715ac 100644
> --- a/arch/powerpc/include/asm/vio.h
> +++ b/arch/powerpc/include/asm/vio.h
> @@ -113,7 +113,7 @@ struct vio_driver {
> const char *name;
> const struct vio_device_id *id_table;
> int (*probe)(struct vio_dev *dev, const struct vio_device_id *id);
> - int (*remove)(struct vio_dev *dev);
> + void (*remove)(struct vio_dev *dev);
> /* A driver must have a get_desired_dma() function to
>* be loaded in a CMO environment if it uses DMA.
>*/
> diff --git a/arch/powerpc/platforms/pseries/vio.c
> b/arch/powerpc/platforms/pseries/vio.c
> index b2797cfe4e2b..9cb4fc839fd5 100644
> --- a/arch/powerpc/platforms/pseries/vio.c
> +++ b/arch/powerpc/platforms/pseries/vio.c
> @@ -1261,7 +1261,6 @@ static int vio_bus_remove(struct device *dev)
> struct vio_dev *viodev = to_vio_dev(dev);
> struct vio_driver *viodrv = to_vio_driver(dev->driver);
> struct device *devptr;
> - int ret = 1;
>
> /*
>* Hold a reference to the device after the remove function is called
> @@ -1270,13 +1269,13 @@ static int vio_bus_remove(struct device *dev)
> devptr = get_device(dev);
>
> if (viodrv->remove)
> - ret = viodrv->remove(viodev);
> + viodrv->remove(viodev);
>
> - if (!ret && firmware_has_feature(FW_FEATURE_CMO))
> + if (firmware_has_feature(FW_FEATURE_CMO))
> vio_cmo_bus_remove(viodev);
>
> put_device(devptr);
> - return ret;
> + return 0;
> }
>
> /**
> diff --git a/arch/sparc/include/asm/vio.h b/arch/sparc/include/asm/vio.h
> index 059f0eb678e0..8a1a83bbb6d5 100644
> --- a/arch/sparc/include/asm/vio.h
> +++ b/arch/sparc/include/asm/vio.h
> @@ -362,7 +362,7 @@ struct vio_driver {
> struct list_headnode;
> const struct vio_device_id *id_table;
> int (*probe)(struct vio_dev *dev, const struct vio_device_id *id);
> - int (*remove)(struct vio_dev *dev);
> + void (*remove)(struct vio_dev *dev);
> void (*shutdown)(struct vio_dev *dev);
> unsigned long driver_data;
> struct
Re: [PATCH] vio: make remove callback return void
Hello Sukadev, On 1/28/21 8:07 PM, Sukadev Bhattiprolu wrote: Slightly off-topic, should ndo_stop() also return a void? Its return value seems to be mostly ignored and [...] I don't know enough about the network stack to tell. Probably it's a good idea to start a separate thread for this and address this to the netdev list only. Best regards Uwe OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH] vio: make remove callback return void
Uwe Kleine-König [u...@kleine-koenig.org] wrote: > 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 vio_driver::remove() return > void, too. All users already unconditionally return 0, this commit makes > it obvious that returning an error code is a bad idea and makes it > obvious for future driver authors that returning an error code isn't > intended. Slightly off-topic, should ndo_stop() also return a void? Its return value seems to be mostly ignored and __dev_close_many() has: /* * Call the device specific close. This cannot fail. * Only if device is UP * * We allow it to be called even after a DETACH hot-plug * event. */ if (ops->ndo_stop) ops->ndo_stop(dev); Sukadev
[PATCH] vio: 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 vio_driver::remove() return
void, too. All users already unconditionally return 0, this commit makes
it obvious that returning an error code is a bad idea and makes it
obvious for future driver authors that returning an error code isn't
intended.
Note there are two nominally different implementations for a vio bus:
one in arch/sparc/kernel/vio.c and the other in
arch/powerpc/platforms/pseries/vio.c. I didn't care to check which
driver is using which of these busses (or if even some of them can be
used with both) and simply adapt all drivers and the two bus codes in
one go.
Note that for the powerpc implementation there is a semantical change:
Before this patch for a device that was bound to a driver without a
remove callback vio_cmo_bus_remove(viodev) wasn't called. As the device
core still considers the device unbound after vio_bus_remove() returns
calling this unconditionally is the consistent behaviour which is
implemented here.
Signed-off-by: Uwe Kleine-König
---
Hello,
note that this change depends on
https://lore.kernel.org/r/20210121062005.53271-1-...@linux.ibm.com which removes
an (ignored) return -EBUSY in drivers/net/ethernet/ibm/ibmvnic.c.
I don't know when/if this latter patch will be applied, so it might take
some time until my patch can go in.
Best regards
Uwe
arch/powerpc/include/asm/vio.h | 2 +-
arch/powerpc/platforms/pseries/vio.c | 7 +++
arch/sparc/include/asm/vio.h | 2 +-
arch/sparc/kernel/ds.c | 6 --
arch/sparc/kernel/vio.c | 4 ++--
drivers/block/sunvdc.c | 3 +--
drivers/char/hw_random/pseries-rng.c | 3 +--
drivers/char/tpm/tpm_ibmvtpm.c | 4 +---
drivers/crypto/nx/nx-842-pseries.c | 4 +---
drivers/crypto/nx/nx.c | 4 +---
drivers/misc/ibmvmc.c| 4 +---
drivers/net/ethernet/ibm/ibmveth.c | 4 +---
drivers/net/ethernet/ibm/ibmvnic.c | 4 +---
drivers/net/ethernet/sun/ldmvsw.c| 4 +---
drivers/net/ethernet/sun/sunvnet.c | 3 +--
drivers/scsi/ibmvscsi/ibmvfc.c | 3 +--
drivers/scsi/ibmvscsi/ibmvscsi.c | 4 +---
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 4 +---
drivers/tty/hvc/hvcs.c | 3 +--
drivers/tty/vcc.c| 4 +---
20 files changed, 22 insertions(+), 54 deletions(-)
diff --git a/arch/powerpc/include/asm/vio.h b/arch/powerpc/include/asm/vio.h
index 0cf52746531b..721c0d6715ac 100644
--- a/arch/powerpc/include/asm/vio.h
+++ b/arch/powerpc/include/asm/vio.h
@@ -113,7 +113,7 @@ struct vio_driver {
const char *name;
const struct vio_device_id *id_table;
int (*probe)(struct vio_dev *dev, const struct vio_device_id *id);
- int (*remove)(struct vio_dev *dev);
+ void (*remove)(struct vio_dev *dev);
/* A driver must have a get_desired_dma() function to
* be loaded in a CMO environment if it uses DMA.
*/
diff --git a/arch/powerpc/platforms/pseries/vio.c
b/arch/powerpc/platforms/pseries/vio.c
index b2797cfe4e2b..9cb4fc839fd5 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -1261,7 +1261,6 @@ static int vio_bus_remove(struct device *dev)
struct vio_dev *viodev = to_vio_dev(dev);
struct vio_driver *viodrv = to_vio_driver(dev->driver);
struct device *devptr;
- int ret = 1;
/*
* Hold a reference to the device after the remove function is called
@@ -1270,13 +1269,13 @@ static int vio_bus_remove(struct device *dev)
devptr = get_device(dev);
if (viodrv->remove)
- ret = viodrv->remove(viodev);
+ viodrv->remove(viodev);
- if (!ret && firmware_has_feature(FW_FEATURE_CMO))
+ if (firmware_has_feature(FW_FEATURE_CMO))
vio_cmo_bus_remove(viodev);
put_device(devptr);
- return ret;
+ return 0;
}
/**
diff --git a/arch/sparc/include/asm/vio.h b/arch/sparc/include/asm/vio.h
index 059f0eb678e0..8a1a83bbb6d5 100644
--- a/arch/sparc/include/asm/vio.h
+++ b/arch/sparc/include/asm/vio.h
@@ -362,7 +362,7 @@ struct vio_driver {
struct list_headnode;
const struct vio_device_id *id_table;
int (*probe)(struct vio_dev *dev, const struct vio_device_id *id);
- int (*remove)(struct vio_dev *dev);
+ void (*remove)(struct vio_dev *dev);
void (*shutdown)(struct vio_dev *dev);
unsigned long driver_data;
struct device_driverdriver;
diff --git a/arch/sparc/kernel/ds.c b/arch/sparc/kernel/ds.c
index 522e5b51050c..4a5bdb0df779 100644
--- a/arch/sparc/kernel/ds.c
+++ b/arch/sparc/kernel/ds.c
@@ -1236,11 +1236,6 @@ static int ds_probe(struct vio_dev
