Kalle Valo <kvalo@xxxxxxxxxx> writes: > Toke Høiland-Jørgensen <toke@xxxxxxxxxx> writes: > >> Felix Fietkau <nbd@xxxxxxxx> writes: >> >>> On 07.08.24 22:07, Rosen Penev wrote: >>>> On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@xxxxxxxx> wrote: >>>>> >>>>> On 07.08.24 20:52, Rosen Penev wrote: >>>>> > On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@xxxxxxxx> 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@xxxxxxxxx> >>>>> >> > --- >>>>> >> > 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
