Re: [PATCH v1] drivers: make struct device_driver::remove return void

[Uwe Kleine-König]

Hello Greg,

On Tue, Nov 10, 2020 at 09:31:06PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 10, 2020 at 04:07:23PM +0100, Uwe Kleine-König wrote:
> > The driver core doesn't check the return value of the remove callback
> > because there is only little software can do when hardware disappears.
> > 
> > So change the callback to not return a value at all and adapt all users.
> > The motivation for this change is that some driver authors have a
> > misconception about failures in the remove callback. Making it void
> > makes it pretty obvious that there is no error handling to be expected.
> > 
> > Most drivers were easy to convert as they returned 0 unconditionally, I
> > added a warning to code paths that returned an error code (that was
> > ignored already before).
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > ---
> > Hello,
> > 
> > I expect that there are still a few driver that need adaption as I only
> > build tested allmodconfig on ARM and amd64.
> > 
> > Best regards
> > Uwe
> > 
> >  drivers/acpi/processor_driver.c         | 10 ++++------
> >  drivers/amba/bus.c                      |  7 ++++---
> >  drivers/base/platform.c                 | 13 +++++++------
> >  drivers/bus/fsl-mc/fsl-mc-bus.c         |  7 ++-----
> >  drivers/bus/mhi/core/init.c             |  6 ++----
> >  drivers/char/hw_random/optee-rng.c      |  4 +---
> >  drivers/char/tpm/tpm_ftpm_tee.c         |  8 ++++----
> >  drivers/firmware/broadcom/tee_bnxt_fw.c |  4 +---
> >  drivers/fsi/fsi-master-hub.c            |  4 +---
> >  drivers/fsi/fsi-sbefifo.c               |  4 +---
> >  drivers/fsi/fsi-scom.c                  |  4 +---
> >  drivers/gpu/drm/drm_mipi_dsi.c          |  7 +++++--
> >  drivers/gpu/host1x/bus.c                | 11 +++++++----
> >  drivers/greybus/core.c                  |  4 +---
> >  drivers/hsi/clients/cmt_speech.c        |  4 +---
> >  drivers/hsi/clients/hsi_char.c          |  4 +---
> >  drivers/hsi/clients/nokia-modem.c       |  4 +---
> >  drivers/hsi/clients/ssi_protocol.c      |  4 +---
> >  drivers/i2c/busses/i2c-fsi.c            |  4 +---
> >  drivers/input/rmi4/rmi_bus.c            |  4 +---
> >  drivers/input/rmi4/rmi_driver.c         |  4 +---
> >  drivers/input/touchscreen/wm97xx-core.c |  7 +++----
> >  drivers/mfd/ucb1400_core.c              |  3 +--
> >  drivers/net/ethernet/3com/3c509.c       |  5 ++---
> >  drivers/net/ethernet/3com/3c59x.c       |  3 +--
> >  drivers/net/ethernet/dec/tulip/de4x5.c  |  4 +---
> >  drivers/net/fddi/defxx.c                |  5 ++---
> >  drivers/net/phy/mdio_device.c           |  4 +---
> >  drivers/net/phy/phy_device.c            |  4 +---
> >  drivers/pci/pcie/portdrv_core.c         |  5 ++---
> >  drivers/scsi/advansys.c                 |  3 +--
> >  drivers/scsi/aha1740.c                  |  4 +---
> >  drivers/scsi/aic7xxx/aic7770_osm.c      |  3 +--
> >  drivers/scsi/ch.c                       |  3 +--
> >  drivers/scsi/sd.c                       |  6 ++----
> >  drivers/scsi/ses.c                      |  3 +--
> >  drivers/scsi/sim710.c                   |  3 +--
> >  drivers/scsi/sr.c                       |  6 ++----
> >  drivers/scsi/st.c                       |  5 ++---
> >  drivers/siox/siox-core.c                |  6 ++++--
> >  drivers/soundwire/bus_type.c            | 13 +++++++------
> >  drivers/spi/spi.c                       |  8 +++++---
> >  drivers/usb/core/driver.c               |  7 ++-----
> >  drivers/visorbus/visorbus_main.c        |  5 +----
> >  include/linux/device/driver.h           |  2 +-
> >  sound/core/seq/oss/seq_oss_synth.c      |  6 ++----
> >  sound/core/seq/oss/seq_oss_synth.h      |  2 +-
> >  sound/core/seq/seq_midi.c               |  6 +++---
> >  sound/drivers/opl3/opl3_seq.c           | 10 ++++++----
> >  sound/drivers/opl4/opl4_seq.c           |  9 +++++----
> >  sound/hda/ext/hdac_ext_bus.c            |  9 +++++++--
> >  sound/isa/sb/emu8000_synth.c            |  5 ++---
> >  sound/pci/emu10k1/emu10k1_synth.c       |  5 ++---
> >  sound/pci/hda/hda_bind.c                | 11 +++++++----
> >  54 files changed, 129 insertions(+), 172 deletions(-)
> 
> First off, that's a lot of drivers with a "raw" remove function, why is
> anyone doing that?  It should all be wrapped up in the bus that the
> drivers are on.
> 
> In digging, ugh, looks like there's some sound driver abuse here that
> should be fixed up first, which will cause you to get these "for free",
> and some busses should be fixed up as well.
> 
> This type of patch should only have to bus code, if things are right,
> not individual drivers.  It's not your fault, but fixing that up will
> make this patch easier as it will be in bisectable pieces :)

I don't understand how this gets more bisectable. The desired cleanups
won't look considerably different, will they? Also irrespective of their
order the intermediate steps will build and run just fine?!

I agree that there are quite some strange drivers, but given my limited
time to work on this now (and expecting to have to rework this patch if
I pick it up again for the next or even after-next merge window) I would
like to see this breed in next already now.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature