[PATCH net-next 0/4] Convert to platform remove callback returning void

[PATCH net-next 0/4] Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

the motivation for this series is patch #3, patch #2 is a preparation for it
and patches #1 and #4 are just cleanups that I noticed en passant.

Best regards
Uwe

Uwe Kleine-König (4):
  ath10k: Drop cleaning of driver data from probe error path and remove
  ath10k: Drop checks that are always false
  ath10k: Convert to platform remove callback returning void
  atk10k: Don't opencode ath10k_pci_priv() in ath10k_ahb_priv()

 drivers/net/wireless/ath/ath10k/ahb.c  | 20 +++-----------------
 drivers/net/wireless/ath/ath10k/snoc.c |  8 +++-----
 2 files changed, 6 insertions(+), 22 deletions(-)

base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.39.2

[PATCH net-next 1/4] ath10k: Drop cleaning of driver data from probe error path and remove

[Uwe Kleine-König]

The driver core cares for resetting driver data if probe fails and after
remove. So drop the explicit and duplicate cleanup in the driver's
functions.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/wireless/ath/ath10k/ahb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
index f0c615fa5614..7bb45c66cff7 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -816,7 +816,6 @@ static int ath10k_ahb_probe(struct platform_device *pdev)
 
 err_core_destroy:
 	ath10k_core_destroy(ar);
-	platform_set_drvdata(pdev, NULL);
 
 	return ret;
 }
@@ -845,8 +844,6 @@ static int ath10k_ahb_remove(struct platform_device *pdev)
 	ath10k_ahb_resource_deinit(ar);
 	ath10k_core_destroy(ar);
 
-	platform_set_drvdata(pdev, NULL);
-
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH net-next 1/4] ath10k: Drop cleaning of driver data from probe error path and remove

[Simon Horman]

On Thu, Jun 01, 2023 at 10:25:53AM +0200, Uwe Kleine-König wrote:
> The driver core cares for resetting driver data if probe fails and after
> remove. So drop the explicit and duplicate cleanup in the driver's
> functions.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 1/4] ath10k: Drop cleaning of driver data from probe error path and remove

[Kalle Valo]

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> wrote:

> The driver core cares for resetting driver data if probe fails and after
> remove. So drop the explicit and duplicate cleanup in the driver's
> functions.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

4 patches applied to ath-next branch of ath.git, thanks.

ec3b1ce2ca34 wifi: ath10k: Drop cleaning of driver data from probe error path and remove
fad5ac80dfa5 wifi: ath10k: Drop checks that are always false
d457bff27633 wifi: ath10k: Convert to platform remove callback returning void
6358b1037157 wifi: atk10k: Don't opencode ath10k_pci_priv() in ath10k_ahb_priv()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230601082556.2738446-2-u.kleine-koenig@pengutronix.de/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH net-next 2/4] ath10k: Drop checks that are always false

[Uwe Kleine-König]

platform_get_drvdata() cannot return NULL as the probe function calls
platform_set_drvdata() with a non-NULL argument or returns with a failure.
In the first case, platform_get_drvdata() returns this non-NULL value and
in the second the remove callback isn't called at all.

ath10k_ahb_priv() cannot return NULL and ar_ahb is unused after the check
anyhow.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/wireless/ath/ath10k/ahb.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
index 7bb45c66cff7..fffdbad75074 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -823,15 +823,6 @@ static int ath10k_ahb_probe(struct platform_device *pdev)
 static int ath10k_ahb_remove(struct platform_device *pdev)
 {
 	struct ath10k *ar = platform_get_drvdata(pdev);
-	struct ath10k_ahb *ar_ahb;
-
-	if (!ar)
-		return -EINVAL;
-
-	ar_ahb = ath10k_ahb_priv(ar);
-
-	if (!ar_ahb)
-		return -EINVAL;
 
 	ath10k_dbg(ar, ATH10K_DBG_AHB, "ahb remove\n");
 
-- 
2.39.2

Re: [PATCH net-next 2/4] ath10k: Drop checks that are always false

[Simon Horman]

On Thu, Jun 01, 2023 at 10:25:54AM +0200, Uwe Kleine-König wrote:
> platform_get_drvdata() cannot return NULL as the probe function calls
> platform_set_drvdata() with a non-NULL argument or returns with a failure.
> In the first case, platform_get_drvdata() returns this non-NULL value and
> in the second the remove callback isn't called at all.
> 
> ath10k_ahb_priv() cannot return NULL and ar_ahb is unused after the check
> anyhow.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next 3/4] ath10k: 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 (mostly) ignored
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.

Both ath10k platform drivers return zero unconditionally in their remove
callback, so they can be trivially converted to use .remove_new().

Also fix on of the more offending whitespace issues in the definition
of ath10k_snoc_driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/wireless/ath/ath10k/ahb.c  | 6 ++----
 drivers/net/wireless/ath/ath10k/snoc.c | 8 +++-----
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
index fffdbad75074..632da4c5e5da 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -820,7 +820,7 @@ static int ath10k_ahb_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int ath10k_ahb_remove(struct platform_device *pdev)
+static void ath10k_ahb_remove(struct platform_device *pdev)
 {
 	struct ath10k *ar = platform_get_drvdata(pdev);
 
@@ -834,8 +834,6 @@ static int ath10k_ahb_remove(struct platform_device *pdev)
 	ath10k_ahb_clock_disable(ar);
 	ath10k_ahb_resource_deinit(ar);
 	ath10k_core_destroy(ar);
-
-	return 0;
 }
 
 static struct platform_driver ath10k_ahb_driver = {
@@ -844,7 +842,7 @@ static struct platform_driver ath10k_ahb_driver = {
 		.of_match_table = ath10k_ahb_of_match,
 	},
 	.probe  = ath10k_ahb_probe,
-	.remove = ath10k_ahb_remove,
+	.remove_new = ath10k_ahb_remove,
 };
 
 int ath10k_ahb_init(void)
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 5128a452c65f..26214c00cd0d 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1848,7 +1848,7 @@ static int ath10k_snoc_free_resources(struct ath10k *ar)
 	return 0;
 }
 
-static int ath10k_snoc_remove(struct platform_device *pdev)
+static void ath10k_snoc_remove(struct platform_device *pdev)
 {
 	struct ath10k *ar = platform_get_drvdata(pdev);
 	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
@@ -1861,8 +1861,6 @@ static int ath10k_snoc_remove(struct platform_device *pdev)
 		wait_for_completion_timeout(&ar->driver_recovery, 3 * HZ);
 
 	ath10k_snoc_free_resources(ar);
-
-	return 0;
 }
 
 static void ath10k_snoc_shutdown(struct platform_device *pdev)
@@ -1875,8 +1873,8 @@ static void ath10k_snoc_shutdown(struct platform_device *pdev)
 
 static struct platform_driver ath10k_snoc_driver = {
 	.probe  = ath10k_snoc_probe,
-	.remove = ath10k_snoc_remove,
-	.shutdown =  ath10k_snoc_shutdown,
+	.remove_new = ath10k_snoc_remove,
+	.shutdown = ath10k_snoc_shutdown,
 	.driver = {
 		.name   = "ath10k_snoc",
 		.of_match_table = ath10k_snoc_dt_match,
-- 
2.39.2

Re: [PATCH net-next 3/4] ath10k: Convert to platform remove callback returning void

[Simon Horman]

On Thu, Jun 01, 2023 at 10:25:55AM +0200, Uwe Kleine-König wrote:
> 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 (mostly) ignored
> 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.
> 
> Both ath10k platform drivers return zero unconditionally in their remove
> callback, so they can be trivially converted to use .remove_new().
> 
> Also fix on of the more offending whitespace issues in the definition
> of ath10k_snoc_driver.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next 4/4] atk10k: Don't opencode ath10k_pci_priv() in ath10k_ahb_priv()

[Uwe Kleine-König]

This introduces no changes in the compiled result (tested for an
ARCH=arm allmodconfig build).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/wireless/ath/ath10k/ahb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
index 632da4c5e5da..4a006fb4d424 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -27,7 +27,7 @@ MODULE_DEVICE_TABLE(of, ath10k_ahb_of_match);
 
 static inline struct ath10k_ahb *ath10k_ahb_priv(struct ath10k *ar)
 {
-	return &((struct ath10k_pci *)ar->drv_priv)->ahb[0];
+	return &ath10k_pci_priv(ar)->ahb[0];
 }
 
 static void ath10k_ahb_write32(struct ath10k *ar, u32 offset, u32 value)
-- 
2.39.2

Re: [PATCH net-next 4/4] atk10k: Don't opencode ath10k_pci_priv() in ath10k_ahb_priv()

[Simon Horman]

On Thu, Jun 01, 2023 at 10:25:56AM +0200, Uwe Kleine-König wrote:
> This introduces no changes in the compiled result (tested for an
> ARCH=arm allmodconfig build).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 0/4] Convert to platform remove callback returning void

[Kalle Valo]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> the motivation for this series is patch #3, patch #2 is a preparation for it
> and patches #1 and #4 are just cleanups that I noticed en passant.
>
> Best regards
> Uwe
>
> Uwe Kleine-König (4):
>   ath10k: Drop cleaning of driver data from probe error path and remove
>   ath10k: Drop checks that are always false
>   ath10k: Convert to platform remove callback returning void
>   atk10k: Don't opencode ath10k_pci_priv() in ath10k_ahb_priv()
>
>  drivers/net/wireless/ath/ath10k/ahb.c  | 20 +++-----------------
>  drivers/net/wireless/ath/ath10k/snoc.c |  8 +++-----
>  2 files changed, 6 insertions(+), 22 deletions(-)

ath10k patches go to my ath.git tree, not net-next. Also "wifi:" is
missing from the title but I can add that.

No need to resend because of these.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next 0/4] Convert to platform remove callback returning void

[Uwe Kleine-König]

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

Hello Kalle,

On Thu, Jun 01, 2023 at 12:17:44PM +0300, Kalle Valo wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > the motivation for this series is patch #3, patch #2 is a preparation for it
> > and patches #1 and #4 are just cleanups that I noticed en passant.
> >
> > Best regards
> > Uwe
> >
> > Uwe Kleine-König (4):
> >   ath10k: Drop cleaning of driver data from probe error path and remove
> >   ath10k: Drop checks that are always false
> >   ath10k: Convert to platform remove callback returning void
> >   atk10k: Don't opencode ath10k_pci_priv() in ath10k_ahb_priv()
> >
> >  drivers/net/wireless/ath/ath10k/ahb.c  | 20 +++-----------------
> >  drivers/net/wireless/ath/ath10k/snoc.c |  8 +++-----
> >  2 files changed, 6 insertions(+), 22 deletions(-)
> 
> ath10k patches go to my ath.git tree, not net-next.

This isn't obvious for outsiders. Not sure what can be improved to
make this easier to spot.

> Also "wifi:" is missing from the title but I can add that.

oops, I wondered about that, too, but was sure that I used the prefix
from previous commits. Now that you said it, suddenly all previous
commits have this wifi prefix. Hmm, somebody must have tinkered with my
source tree :-)

Thanks for fixing my misdemeanors,
Uwe

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

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

Re: [PATCH net-next 0/4] Convert to platform remove callback returning void

[Kalle Valo]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> Hello Kalle,
>
> On Thu, Jun 01, 2023 at 12:17:44PM +0300, Kalle Valo wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> 
>> > the motivation for this series is patch #3, patch #2 is a preparation for it
>> > and patches #1 and #4 are just cleanups that I noticed en passant.
>> >
>> > Best regards
>> > Uwe
>> >
>> > Uwe Kleine-König (4):
>> >   ath10k: Drop cleaning of driver data from probe error path and remove
>> >   ath10k: Drop checks that are always false
>> >   ath10k: Convert to platform remove callback returning void
>> >   atk10k: Don't opencode ath10k_pci_priv() in ath10k_ahb_priv()
>> >
>> >  drivers/net/wireless/ath/ath10k/ahb.c  | 20 +++-----------------
>> >  drivers/net/wireless/ath/ath10k/snoc.c |  8 +++-----
>> >  2 files changed, 6 insertions(+), 22 deletions(-)
>> 
>> ath10k patches go to my ath.git tree, not net-next.
>
> This isn't obvious for outsiders. Not sure what can be improved to
> make this easier to spot.

Yeah, you are definitely not the first one who is bitten by this. We do
have the tree documented in MAINTAINERS but that's easy to miss:

QUALCOMM ATHEROS ATH10K WIRELESS DRIVER
M:      Kalle Valo <kvalo@kernel.org>
L:      ath10k@lists.infradead.org
S:      Supported
W:      https://wireless.wiki.kernel.org/en/users/Drivers/ath10k
T:      git git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git
F:      Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
F:      drivers/net/wireless/ath/ath10k/

And a rule of thumb is that all wireless patches do normally go to
wireless or wireless-next trees, and for most active wireless drivers we
have separate trees as well (like ath.git in this case).

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches