[PATCH net-next 00/19] can: Convert to platform remove callback returning void

[PATCH net-next 00/19] can: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

this series convers the drivers below drivers/net/can to the
.remove_new() callback of struct platform_driver(). The motivation is to
make the remove callback less prone for errors and wrong assumptions.
See commit 5c5a7680e67b ("platform: Provide a remove callback that
returns no value") for a more detailed rationale.

All drivers already returned zero unconditionally in their
.remove() callback, so converting them to .remove_new() is trivial.

Best regards
Uwe

Uwe Kleine-König (19):
  can: at91_can: Convert to platform remove callback returning void
  can: bxcan: Convert to platform remove callback returning void
  can: c_can: Convert to platform remove callback returning void
  can: cc770_isa: Convert to platform remove callback returning void
  can: cc770_platform: Convert to platform remove callback returning void
  can: ctucanfd: Convert to platform remove callback returning void
  can: flexcan: Convert to platform remove callback returning void
  can: grcan: Convert to platform remove callback returning void
  can: ifi_canfd: Convert to platform remove callback returning void
  can: janz-ican3: Convert to platform remove callback returning void
  can: m_can: Convert to platform remove callback returning void
  can: mscan/mpc5xxx_can.c -- Convert to platform remove callback returning void
  can: rcar: Convert to platform remove callback returning void
  can: sja1000_isa: Convert to platform remove callback returning void
  can: sja1000_platform: Convert to platform remove callback returning void
  can: softing: Convert to platform remove callback returning void
  can: sun4i_can: Convert to platform remove callback returning void
  can: ti_hecc: Convert to platform remove callback returning void
  can: xilinx: Convert to platform remove callback returning void

 drivers/net/can/at91_can.c                   | 6 ++----
 drivers/net/can/bxcan.c                      | 5 ++---
 drivers/net/can/c_can/c_can_platform.c       | 6 ++----
 drivers/net/can/cc770/cc770_isa.c            | 6 ++----
 drivers/net/can/cc770/cc770_platform.c       | 6 ++----
 drivers/net/can/ctucanfd/ctucanfd_platform.c | 6 ++----
 drivers/net/can/flexcan/flexcan-core.c       | 6 ++----
 drivers/net/can/grcan.c                      | 6 ++----
 drivers/net/can/ifi_canfd/ifi_canfd.c        | 6 ++----
 drivers/net/can/janz-ican3.c                 | 6 ++----
 drivers/net/can/m_can/m_can_platform.c       | 6 ++----
 drivers/net/can/mscan/mpc5xxx_can.c          | 6 ++----
 drivers/net/can/rcar/rcar_can.c              | 5 ++---
 drivers/net/can/rcar/rcar_canfd.c            | 6 ++----
 drivers/net/can/sja1000/sja1000_isa.c        | 6 ++----
 drivers/net/can/sja1000/sja1000_platform.c   | 6 ++----
 drivers/net/can/softing/softing_main.c       | 5 ++---
 drivers/net/can/sun4i_can.c                  | 6 ++----
 drivers/net/can/ti_hecc.c                    | 6 ++----
 drivers/net/can/xilinx_can.c                 | 6 ++----
 20 files changed, 40 insertions(+), 77 deletions(-)


base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.39.2

[PATCH 17/19] can: sun4i_can: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart from
emitting a warning) and this typically results in resource leaks. To improve
here there is a quest to make the remove callback return void. In the first
step of this quest all drivers are converted to .remove_new() which already
returns void. Eventually after all drivers are converted, .remove_new() is
renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/can/sun4i_can.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
index 2b78f9197681..0827830bbf28 100644
--- a/drivers/net/can/sun4i_can.c
+++ b/drivers/net/can/sun4i_can.c
@@ -791,14 +791,12 @@ static const struct of_device_id sun4ican_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, sun4ican_of_match);
 
-static int sun4ican_remove(struct platform_device *pdev)
+static void sun4ican_remove(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
 
 	unregister_netdev(dev);
 	free_candev(dev);
-
-	return 0;
 }
 
 static int sun4ican_probe(struct platform_device *pdev)
@@ -901,7 +899,7 @@ static struct platform_driver sun4i_can_driver = {
 		.of_match_table = sun4ican_of_match,
 	},
 	.probe = sun4ican_probe,
-	.remove = sun4ican_remove,
+	.remove_new = sun4ican_remove,
 };
 
 module_platform_driver(sun4i_can_driver);
-- 
2.39.2

Re: [PATCH 17/19] can: sun4i_can: Convert to platform remove callback returning void

[Jernej Škrabec]

Dne petek, 12. maj 2023 ob 23:27:23 CEST je Uwe Kleine-König napisal(a):
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart from
> emitting a warning) and this typically results in resource leaks. To improve
> here there is a quest to make the remove callback return void. In the first
> step of this quest all drivers are converted to .remove_new() which already
> returns void. Eventually after all drivers are converted, .remove_new() is
> renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

Re: [PATCH 17/19] can: sun4i_can: Convert to platform remove callback returning void

[Gerhard Bertelsmann]

Am 2023-05-12 23:27, schrieb Uwe Kleine-König:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling 
> by
> returning an error code. However the value returned is ignored (apart 
> from
> emitting a warning) and this typically results in resource leaks. To 
> improve
> here there is a quest to make the remove callback return void. In the 
> first
> step of this quest all drivers are converted to .remove_new() which 
> already
> returns void. Eventually after all drivers are converted, .remove_new() 
> is
> renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Gerhard Bertelsmann <info@gerhard-bertelsmann.de>

Thanks Uwe :-)

Re: [PATCH net-next 00/19] can: Convert to platform remove callback returning void

[Simon Horman]

On Fri, May 12, 2023 at 11:27:06PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this series convers the drivers below drivers/net/can to the
> .remove_new() callback of struct platform_driver(). The motivation is to
> make the remove callback less prone for errors and wrong assumptions.
> See commit 5c5a7680e67b ("platform: Provide a remove callback that
> returns no value") for a more detailed rationale.
> 
> All drivers already returned zero unconditionally in their
> .remove() callback, so converting them to .remove_new() is trivial.

Hi Uwe,

I like these changes and they all look good to me.
However, I have a question, perhaps more directed at the netdev
maintainers than yourself.

In principle patch-sets for netdev should not include more than 15 patches.
It's unclear to me if, on the basis of that, this patchset should
be split up. Or if, f.e. given the simple nature of the patches,
an exception applies in this case. Or something else.

I have no fixed opinion on this.
But I feel that the question should be asked.

Link: https://kernel.org/doc/html/v6.1/process/maintainer-netdev.html

...

Re: [PATCH net-next 00/19] can: Convert to platform remove callback returning void

[Simon Horman]

On Tue, May 16, 2023 at 09:31:04AM +0200, Simon Horman wrote:
> On Fri, May 12, 2023 at 11:27:06PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > this series convers the drivers below drivers/net/can to the
> > .remove_new() callback of struct platform_driver(). The motivation is to
> > make the remove callback less prone for errors and wrong assumptions.
> > See commit 5c5a7680e67b ("platform: Provide a remove callback that
> > returns no value") for a more detailed rationale.
> > 
> > All drivers already returned zero unconditionally in their
> > .remove() callback, so converting them to .remove_new() is trivial.
> 
> Hi Uwe,
> 
> I like these changes and they all look good to me.
> However, I have a question, perhaps more directed at the netdev
> maintainers than yourself.
> 
> In principle patch-sets for netdev should not include more than 15 patches.
> It's unclear to me if, on the basis of that, this patchset should
> be split up. Or if, f.e. given the simple nature of the patches,
> an exception applies in this case. Or something else.
> 
> I have no fixed opinion on this.
> But I feel that the question should be asked.
> 
> Link: https://kernel.org/doc/html/v6.1/process/maintainer-netdev.html
> 
> ...

I now realise this series is for can.
Which I assume means the guidance above doesn't apply.

Sorry for the noise.

Re: [PATCH net-next 00/19] can: Convert to platform remove callback returning void

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

On 16.05.2023 10:45:23, Simon Horman wrote:
> On Tue, May 16, 2023 at 09:31:04AM +0200, Simon Horman wrote:
> > On Fri, May 12, 2023 at 11:27:06PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > this series convers the drivers below drivers/net/can to the
> > > .remove_new() callback of struct platform_driver(). The motivation is to
> > > make the remove callback less prone for errors and wrong assumptions.
> > > See commit 5c5a7680e67b ("platform: Provide a remove callback that
> > > returns no value") for a more detailed rationale.
> > > 
> > > All drivers already returned zero unconditionally in their
> > > .remove() callback, so converting them to .remove_new() is trivial.
> > 
> > Hi Uwe,
> > 
> > I like these changes and they all look good to me.
> > However, I have a question, perhaps more directed at the netdev
> > maintainers than yourself.
> > 
> > In principle patch-sets for netdev should not include more than 15 patches.
> > It's unclear to me if, on the basis of that, this patchset should
> > be split up. Or if, f.e. given the simple nature of the patches,
> > an exception applies in this case. Or something else.
> > 
> > I have no fixed opinion on this.
> > But I feel that the question should be asked.
> > 
> > Link: https://kernel.org/doc/html/v6.1/process/maintainer-netdev.html
> > 
> > ...
> 
> I now realise this series is for can.
> Which I assume means the guidance above doesn't apply.
> 
> Sorry for the noise.

That's still a good point, because sooner or later Uwe will probably
also convert the platform drivers to Driver/Network/Ethernet.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 00/19] can: Convert to platform remove callback returning void

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On 16.05.2023 10:54:06, Marc Kleine-Budde wrote:
> On 16.05.2023 10:45:23, Simon Horman wrote:
> > On Tue, May 16, 2023 at 09:31:04AM +0200, Simon Horman wrote:
> > > On Fri, May 12, 2023 at 11:27:06PM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > this series convers the drivers below drivers/net/can to the
> > > > .remove_new() callback of struct platform_driver(). The motivation is to
> > > > make the remove callback less prone for errors and wrong assumptions.
> > > > See commit 5c5a7680e67b ("platform: Provide a remove callback that
> > > > returns no value") for a more detailed rationale.
> > > > 
> > > > All drivers already returned zero unconditionally in their
> > > > .remove() callback, so converting them to .remove_new() is trivial.
> > > 
> > > Hi Uwe,
> > > 
> > > I like these changes and they all look good to me.
> > > However, I have a question, perhaps more directed at the netdev
> > > maintainers than yourself.
> > > 
> > > In principle patch-sets for netdev should not include more than 15 patches.
> > > It's unclear to me if, on the basis of that, this patchset should
> > > be split up. Or if, f.e. given the simple nature of the patches,
> > > an exception applies in this case. Or something else.
> > > 
> > > I have no fixed opinion on this.
> > > But I feel that the question should be asked.
> > > 
> > > Link: https://kernel.org/doc/html/v6.1/process/maintainer-netdev.html
> > > 
> > > ...
> > 
> > I now realise this series is for can.
> > Which I assume means the guidance above doesn't apply.
> > 
> > Sorry for the noise.
> 
> That's still a good point, because sooner or later Uwe will probably
> also convert the platform drivers to Driver/Network/Ethernet.

...in driver/net/ethernet.

(damnyouautocorrect)

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]