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

["Toke Høiland-Jørgensen" <toke@kernel.org>]

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