[PATCH] net: ath9k: use devm for request_irq

[PATCH] net: ath9k: use devm for request_irq

[Rosen Penev]

Avoids having to manually call free_irq. Simplifies code slightly.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
 drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
index 1a6697b6e3b4..29f67ded8fe2 100644
--- a/drivers/net/wireless/ath/ath9k/ahb.c
+++ b/drivers/net/wireless/ath/ath9k/ahb.c
@@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
 	sc->mem = mem;
 	sc->irq = irq;
 
-	ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
+	ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq failed\n");
 		goto err_free_hw;
@@ -127,7 +127,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
 	ret = ath9k_init_device(id->driver_data, sc, &ath_ahb_bus_ops);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to initialize device\n");
-		goto err_irq;
+		goto err_free_hw;
 	}
 
 	ah = sc->sc_ah;
@@ -137,8 +137,6 @@ static int ath_ahb_probe(struct platform_device *pdev)
 
 	return 0;
 
- err_irq:
-	free_irq(irq, sc);
  err_free_hw:
 	ieee80211_free_hw(hw);
 	return ret;
@@ -152,7 +150,6 @@ static void ath_ahb_remove(struct platform_device *pdev)
 		struct ath_softc *sc = hw->priv;
 
 		ath9k_deinit_device(sc);
-		free_irq(sc->irq, sc);
 		ieee80211_free_hw(sc->hw);
 	}
 }
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 1ff53520f0a3..ccf73886199a 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -965,9 +965,9 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 
 	if (!msi_enabled)
-		ret = request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc);
+		ret = devm_request_irq(&pdev->dev, pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc);
 	else
-		ret = request_irq(pdev->irq, ath_isr, 0, "ath9k", sc);
+		ret = devm_request_irq(&pdev->dev, pdev->irq, ath_isr, 0, "ath9k", sc);
 
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq failed\n");
@@ -979,7 +979,7 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ret = ath9k_init_device(id->device, sc, &ath_pci_bus_ops);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize device\n");
-		goto err_init;
+		goto err_irq;
 	}
 
 	sc->sc_ah->msi_enabled = msi_enabled;
@@ -991,8 +991,6 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	return 0;
 
-err_init:
-	free_irq(sc->irq, sc);
 err_irq:
 	ieee80211_free_hw(hw);
 	return ret;
@@ -1006,7 +1004,6 @@ static void ath_pci_remove(struct pci_dev *pdev)
 	if (!is_ath9k_unloaded)
 		sc->sc_ah->ah_flags |= AH_UNPLUGGED;
 	ath9k_deinit_device(sc);
-	free_irq(sc->irq, sc);
 	ieee80211_free_hw(sc->hw);
 }
 
-- 
2.45.2

Re: [PATCH] net: ath9k: use devm for request_irq

[Kalle Valo]

Rosen Penev <rosenp@gmail.com> writes:

> Avoids having to manually call free_irq. Simplifies code slightly.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

The title prefix should be "wifi:". I can fix that, no need to resend
because of this.

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

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

Re: [PATCH] net: ath9k: use devm for request_irq

[Toke Høiland-Jørgensen]

Rosen Penev <rosenp@gmail.com> writes:

> Avoids having to manually call free_irq. Simplifies code slightly.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>

Re: [PATCH] net: ath9k: use devm for request_irq

[Kalle Valo]

Rosen Penev <rosenp@gmail.com> wrote:

> Avoids having to manually call free_irq(). Simplifies code slightly.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

92da4ce847bc wifi: ath9k: use devm for request_irq()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240731210243.7467-1-rosenp@gmail.com/

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

Re: [PATCH] net: ath9k: use devm for request_irq

[Felix Fietkau]

On 31.07.24 23:02, Rosen Penev wrote:
> Avoids having to manually call free_irq. Simplifies code slightly.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
>   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
>   2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
> index 1a6697b6e3b4..29f67ded8fe2 100644
> --- a/drivers/net/wireless/ath/ath9k/ahb.c
> +++ b/drivers/net/wireless/ath/ath9k/ahb.c
> @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>   	sc->mem = mem;
>   	sc->irq = irq;
>   
> -	ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
> +	ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
Sorry for the late response, but I think this patch is wrong any may 
need to be reverted. If there is an error during probe, and the IRQ 
fires for some reason, there could be an use-after-free bug when the IRQ 
handler accesses the data in sc.
The explicit freq_irq calls were preventing that from happening.

- Felix

Re: [PATCH] net: ath9k: use devm for request_irq

[Rosen Penev]

On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
>
> On 31.07.24 23:02, Rosen Penev wrote:
> > Avoids having to manually call free_irq. Simplifies code slightly.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
> >   2 files changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
> > index 1a6697b6e3b4..29f67ded8fe2 100644
> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
> >       sc->mem = mem;
> >       sc->irq = irq;
> >
> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
> Sorry for the late response, but I think this patch is wrong any may
> need to be reverted. If there is an error during probe, and the IRQ
> fires for some reason, there could be an use-after-free bug when the IRQ
> handler accesses the data in sc.
> The explicit freq_irq calls were preventing that from happening.
How about keeping the devm variant and replacing free_irq with
devm_free_irq in probe?
>
> - Felix
>

Re: [PATCH] net: ath9k: use devm for request_irq

[Felix Fietkau]

On 07.08.24 20:52, Rosen Penev wrote:
> On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
>>
>> On 31.07.24 23:02, Rosen Penev wrote:
>> > Avoids having to manually call free_irq. Simplifies code slightly.
>> >
>> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>> > ---
>> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
>> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
>> >   2 files changed, 5 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
>> > index 1a6697b6e3b4..29f67ded8fe2 100644
>> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
>> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
>> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>> >       sc->mem = mem;
>> >       sc->irq = irq;
>> >
>> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>> Sorry for the late response, but I think this patch is wrong any may
>> need to be reverted. If there is an error during probe, and the IRQ
>> fires for some reason, there could be an use-after-free bug when the IRQ
>> handler accesses the data in sc.
>> The explicit freq_irq calls were preventing that from happening.
> How about keeping the devm variant and replacing free_irq with
> devm_free_irq in probe?

If you do that, then using the devm variant is completely pointless.
I think a full revert is the best option.

- Felix

Re: [PATCH] net: ath9k: use devm for request_irq

[Rosen Penev]

On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@nbd.name> wrote:
>
> On 07.08.24 20:52, Rosen Penev wrote:
> > On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
> >>
> >> On 31.07.24 23:02, Rosen Penev wrote:
> >> > Avoids having to manually call free_irq. Simplifies code slightly.
> >> >
> >> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> >> > ---
> >> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
> >> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
> >> >   2 files changed, 5 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
> >> > index 1a6697b6e3b4..29f67ded8fe2 100644
> >> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
> >> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
> >> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
> >> >       sc->mem = mem;
> >> >       sc->irq = irq;
> >> >
> >> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
> >> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
> >> Sorry for the late response, but I think this patch is wrong any may
> >> need to be reverted. If there is an error during probe, and the IRQ
> >> fires for some reason, there could be an use-after-free bug when the IRQ
> >> handler accesses the data in sc.
> >> The explicit freq_irq calls were preventing that from happening.
> > How about keeping the devm variant and replacing free_irq with
> > devm_free_irq in probe?
>
> If you do that, then using the devm variant is completely pointless.
> I think a full revert is the best option.
OTOH it still allows removing free_irq from _remove, but I see your point.
>
> - Felix

Re: [PATCH] net: ath9k: use devm for request_irq

[Felix Fietkau]

On 07.08.24 22:07, Rosen Penev wrote:
> On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@nbd.name> wrote:
>>
>> On 07.08.24 20:52, Rosen Penev wrote:
>> > On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
>> >>
>> >> On 31.07.24 23:02, Rosen Penev wrote:
>> >> > Avoids having to manually call free_irq. Simplifies code slightly.
>> >> >
>> >> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>> >> > ---
>> >> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
>> >> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
>> >> >   2 files changed, 5 insertions(+), 11 deletions(-)
>> >> >
>> >> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
>> >> > index 1a6697b6e3b4..29f67ded8fe2 100644
>> >> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
>> >> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
>> >> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>> >> >       sc->mem = mem;
>> >> >       sc->irq = irq;
>> >> >
>> >> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>> >> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>> >> Sorry for the late response, but I think this patch is wrong any may
>> >> need to be reverted. If there is an error during probe, and the IRQ
>> >> fires for some reason, there could be an use-after-free bug when the IRQ
>> >> handler accesses the data in sc.
>> >> The explicit freq_irq calls were preventing that from happening.
>> > How about keeping the devm variant and replacing free_irq with
>> > devm_free_irq in probe?
>>
>> If you do that, then using the devm variant is completely pointless.
>> I think a full revert is the best option.
> OTOH it still allows removing free_irq from _remove, but I see your point.

No, because you'd have the same use-after-free bug there as well.

- Felix

Re: [PATCH] net: ath9k: use devm for request_irq

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> On 07.08.24 22:07, Rosen Penev wrote:
>> On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@nbd.name> wrote:
>>>
>>> On 07.08.24 20:52, Rosen Penev wrote:
>>> > On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
>>> >>
>>> >> On 31.07.24 23:02, Rosen Penev wrote:
>>> >> > Avoids having to manually call free_irq. Simplifies code slightly.
>>> >> >
>>> >> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>> >> > ---
>>> >> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
>>> >> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
>>> >> >   2 files changed, 5 insertions(+), 11 deletions(-)
>>> >> >
>>> >> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
>>> >> > index 1a6697b6e3b4..29f67ded8fe2 100644
>>> >> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
>>> >> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
>>> >> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>>> >> >       sc->mem = mem;
>>> >> >       sc->irq = irq;
>>> >> >
>>> >> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>> >> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>> >> Sorry for the late response, but I think this patch is wrong any may
>>> >> need to be reverted. If there is an error during probe, and the IRQ
>>> >> fires for some reason, there could be an use-after-free bug when the IRQ
>>> >> handler accesses the data in sc.
>>> >> The explicit freq_irq calls were preventing that from happening.
>>> > How about keeping the devm variant and replacing free_irq with
>>> > devm_free_irq in probe?
>>>
>>> If you do that, then using the devm variant is completely pointless.
>>> I think a full revert is the best option.
>> OTOH it still allows removing free_irq from _remove, but I see your point.
>
> No, because you'd have the same use-after-free bug there as well.

Alright, let's revert. Kalle, can you just do the revert, or should I
send a patch for it?

-Toke

Re: [PATCH] net: ath9k: use devm for request_irq

[Kalle Valo]

Toke Høiland-Jørgensen <toke@kernel.org> writes:

> Felix Fietkau <nbd@nbd.name> writes:
>
>> On 07.08.24 22:07, Rosen Penev wrote:
>>> On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@nbd.name> wrote:
>>>>
>>>> On 07.08.24 20:52, Rosen Penev wrote:
>>>> > On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
>>>> >>
>>>> >> On 31.07.24 23:02, Rosen Penev wrote:
>>>> >> > Avoids having to manually call free_irq. Simplifies code slightly.
>>>> >> >
>>>> >> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>>> >> > ---
>>>> >> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
>>>> >> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
>>>> >> >   2 files changed, 5 insertions(+), 11 deletions(-)
>>>> >> >
>>>> >> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
>>>> >> > index 1a6697b6e3b4..29f67ded8fe2 100644
>>>> >> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
>>>> >> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
>>>> >> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>>>> >> >       sc->mem = mem;
>>>> >> >       sc->irq = irq;
>>>> >> >
>>>> >> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>>> >> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>>> >> Sorry for the late response, but I think this patch is wrong any may
>>>> >> need to be reverted. If there is an error during probe, and the IRQ
>>>> >> fires for some reason, there could be an use-after-free bug when the IRQ
>>>> >> handler accesses the data in sc.
>>>> >> The explicit freq_irq calls were preventing that from happening.
>>>> > How about keeping the devm variant and replacing free_irq with
>>>> > devm_free_irq in probe?
>>>>
>>>> If you do that, then using the devm variant is completely pointless.
>>>> I think a full revert is the best option.
>>> OTOH it still allows removing free_irq from _remove, but I see your point.
>>
>> No, because you'd have the same use-after-free bug there as well.
>
> Alright, let's revert. Kalle, can you just do the revert, or should I
> send a patch for it?

Thanks, the best is to send a patch.

But honestly more and more I'm starting to think that we should just
reject all these "drive-by cleanups". We have better things to do than
fixing unnecessary their bugs. Thoughts?

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

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

Re: [PATCH] net: ath9k: use devm for request_irq

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@kernel.org> writes:

> Toke Høiland-Jørgensen <toke@kernel.org> writes:
>
>> Felix Fietkau <nbd@nbd.name> writes:
>>
>>> On 07.08.24 22:07, Rosen Penev wrote:
>>>> On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@nbd.name> wrote:
>>>>>
>>>>> On 07.08.24 20:52, Rosen Penev wrote:
>>>>> > On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
>>>>> >>
>>>>> >> On 31.07.24 23:02, Rosen Penev wrote:
>>>>> >> > Avoids having to manually call free_irq. Simplifies code slightly.
>>>>> >> >
>>>>> >> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>>>> >> > ---
>>>>> >> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
>>>>> >> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
>>>>> >> >   2 files changed, 5 insertions(+), 11 deletions(-)
>>>>> >> >
>>>>> >> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
>>>>> >> > index 1a6697b6e3b4..29f67ded8fe2 100644
>>>>> >> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
>>>>> >> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
>>>>> >> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>>>>> >> >       sc->mem = mem;
>>>>> >> >       sc->irq = irq;
>>>>> >> >
>>>>> >> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>>>> >> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>>>> >> Sorry for the late response, but I think this patch is wrong any may
>>>>> >> need to be reverted. If there is an error during probe, and the IRQ
>>>>> >> fires for some reason, there could be an use-after-free bug when the IRQ
>>>>> >> handler accesses the data in sc.
>>>>> >> The explicit freq_irq calls were preventing that from happening.
>>>>> > How about keeping the devm variant and replacing free_irq with
>>>>> > devm_free_irq in probe?
>>>>>
>>>>> If you do that, then using the devm variant is completely pointless.
>>>>> I think a full revert is the best option.
>>>> OTOH it still allows removing free_irq from _remove, but I see your point.
>>>
>>> No, because you'd have the same use-after-free bug there as well.
>>
>> Alright, let's revert. Kalle, can you just do the revert, or should I
>> send a patch for it?
>
> Thanks, the best is to send a patch.

Alright, will do.

> But honestly more and more I'm starting to think that we should just
> reject all these "drive-by cleanups". We have better things to do than
> fixing unnecessary their bugs. Thoughts?

Hmm, yeah, maybe. I do kinda like the fact that people send patches to
improve small things, though. We all started out as new to the kernel,
and I appreciate the fact that people try to improve our "commons" in
this way even if it's small things.

I do try to be critical of things that can break stuff before ack'ing
these fixes, but I'll admit that it seems like I don't have that great
of a track record for judging "correct" in this context (cf this one,
and that debugfs regression). So I guess you're right that I should at
least raise the bar somewhat; will try to recalibrate and say no more :)

-Toke

Re: [PATCH] net: ath9k: use devm for request_irq

[Kalle Valo]

Toke Høiland-Jørgensen <toke@kernel.org> writes:

>> But honestly more and more I'm starting to think that we should just
>> reject all these "drive-by cleanups". We have better things to do than
>> fixing unnecessary their bugs. Thoughts?
>
> Hmm, yeah, maybe. I do kinda like the fact that people send patches to
> improve small things, though. We all started out as new to the kernel,
> and I appreciate the fact that people try to improve our "commons" in
> this way even if it's small things.

Yeah, you have a point. It's just that the extra work from cleanups
feels so unnecessary compared to the practical benefits. And most of the
time we don't hear from these people ever again, that's why I call them
"drive-by cleanup".

> I do try to be critical of things that can break stuff before ack'ing
> these fixes, but I'll admit that it seems like I don't have that great
> of a track record for judging "correct" in this context (cf this one,
> and that debugfs regression). So I guess you're right that I should at
> least raise the bar somewhat; will try to recalibrate and say no more :)

You are doing a great job :) Nobody can catch all bugs in review, I
would say there is a small percentage (5%?) of all cleanup patches that
cause issues. Though it would be cool to see some real statistics.

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

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

Re: [PATCH] net: ath9k: use devm for request_irq

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@kernel.org> writes:

> Toke Høiland-Jørgensen <toke@kernel.org> writes:
>
>>> But honestly more and more I'm starting to think that we should just
>>> reject all these "drive-by cleanups". We have better things to do than
>>> fixing unnecessary their bugs. Thoughts?
>>
>> Hmm, yeah, maybe. I do kinda like the fact that people send patches to
>> improve small things, though. We all started out as new to the kernel,
>> and I appreciate the fact that people try to improve our "commons" in
>> this way even if it's small things.
>
> Yeah, you have a point. It's just that the extra work from cleanups
> feels so unnecessary compared to the practical benefits. And most of the
> time we don't hear from these people ever again, that's why I call them
> "drive-by cleanup".

Sure, I share the frustration, and not everyone turns into regular
contributors. But I like to think that even those who don't get
something else out of it at least. And on our side I guess there's a
balance to be struck between being welcoming and not expending too much
effort on it :)

>> I do try to be critical of things that can break stuff before ack'ing
>> these fixes, but I'll admit that it seems like I don't have that great
>> of a track record for judging "correct" in this context (cf this one,
>> and that debugfs regression). So I guess you're right that I should at
>> least raise the bar somewhat; will try to recalibrate and say no more :)
>
> You are doing a great job :) Nobody can catch all bugs in review, I
> would say there is a small percentage (5%?) of all cleanup patches that
> cause issues. Though it would be cool to see some real statistics.

Thanks! Yeah, would love to see some statistics, but I suppose it's not
trivial to identity "cleanup patches" in the first place in a way that
can be automated. Maybe something to poke at sometime one has time to
spare (ha!) or needs a break from the regular drudgery :)

-Toke