On Fri, Mar 28, 2025 at 09:39:08PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki(a)intel.com>
>
> Since acpi_processor_notify() can be called before registering a cpufreq
> driver or even in cases when a cpufreq driver is not registered at all,
> cpufreq_update_limits() needs to check if a cpufreq driver is present
> and prevent it from being unregistered.
>
> For this purpose, make it call cpufreq_cpu_get() to obtain a cpufreq
> policy pointer for the given CPU and reference count the corresponding
> policy object, if present.
>
> Fixes: 5a25e3f7cc53 ("cpufreq: intel_pstate: Driver-specific handling of _PPC updates")
> Closes: https://lore.kernel.org/linux-acpi/Z-ShAR59cTow0KcR@mail-itl
> Reporetd-by: Marek Marczykowski-Górecki <marmarek(a)invisiblethingslab.com>
> Cc: All applicable <stable(a)vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki(a)intel.com>
It looks like this patch is missing in stable branches.
> ---
> drivers/cpufreq/cpufreq.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2781,6 +2781,12 @@
> */
> void cpufreq_update_limits(unsigned int cpu)
> {
> + struct cpufreq_policy *policy __free(put_cpufreq_policy);
> +
> + policy = cpufreq_cpu_get(cpu);
> + if (!policy)
> + return;
> +
> if (cpufreq_driver->update_limits)
> cpufreq_driver->update_limits(cpu);
> else
>
>
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
On Tue, Apr 15, 2025 at 08:28:18PM +0800, Wentao Liang wrote:
> >Again, I think this whole "hal wrapper" should be removed instead, and
> >not papered over like this. If you dig deep enough, it all boils down
> >to a call to sdio_readb(), which is an 8 bit read, so the alignment
> >issues are not a problem, and if an error happens the proper error value>
> >is returned from that saying what happened. Why not work on that like I
> >recommended? That would allow for at least 3, if not more, layers of
> >indirection to be removed from this driver, making it more easy to
> >understand and maintain over time.
>
> Thanks for the guidance and detailed suggestion. But remove the whole
> "hal wrapper" layer is beyond my capability. Perhaps this refactoring
> work would be better handled by someone with deeper knowledge of the
> driver codebase. The changes would ripple through several layers, and
> I'm not entirely confident about implementing them correctly. The
> current patch might serve as a reasonable stopgap solution.
Try it, it should be a "one step at a time" of unwinding the mess that
the codebase is in. Shouldn't be that hard, and will give you lots of
good things to work on.
I don't want to take a "stopgap solution", sorry, I would rather take
the real solution, for obvious reasons.
thanks,
greg k-h
On Fri, 2025-04-11 at 13:05 +0200, Christian König wrote:
> Am 11.04.25 um 11:29 schrieb Philipp Stanner:
>
> > [SNIP]
> >
> > It could be, however, that at the same moment
> > nouveau_fence_signal() is
> > removing that entry, holding the appropriate lock.
> >
> > So we have a race. Again.
> >
>
> Ah, yes of course. If signaled is called with or without the lock is
> actually undetermined.
>
>
> >
> > You see, fixing things in Nouveau is difficult :)
> > It gets more difficult if you want to clean it up "properly", so it
> > conforms to rules such as those from dma_fence.
> >
> > I have now provided two fixes that both work, but you are not
> > satisfied
> > with from the dma_fence-maintainer's perspective. I understand
> > that,
> > but please also understand that it's actually not my primary task
> > to
> > work on Nouveau. I just have to fix this bug to move on with my
> > scheduler work.
> >
>
> Well I'm happy with whatever solution as long as it works, but as
> far as I can see the approach with the callback simply doesn't.
>
> You just can't drop the fence reference for the list from the
> callback.
>
>
> >
> > So if you have another idea, feel free to share it. But I'd like to
> > know how we can go on here.
> >
>
> Well the fence code actually works, doesn't it? The problem is
> rather that setting the error throws a warning because it doesn't
> expect signaled fences on the pending list.
>
> Maybe we should fix that instead.
The fence code works as the author intended, but I would be happy if it
were more explicitly documented.
Regarding the WARN_ON: It occurs in dma_fence_set_error() because there
is an attempt to set an error code on a signaled fence. I don't think
that should be "fixed", it works as intended: You must not set an error
code of a fence that was already signaled.
The reason seems to be that once a fence is signaled, a third party
might evaluate the error code.
But I think this wasn't wat you meant with "fix".
In any case, there must not be signaled fences in nouveau's pending-
list. They must be removed immediately once they signal, and this must
not race.
>
>
> >
> > I'm running out of ideas. What I'm wondering if we couldn't just
> > remove
> > performance hacky fastpath functions such as
> > nouveau_fence_is_signaled() completely. It seems redundant to me.
> >
>
> That would work for me as well.
I'll test this approach. Seems a bit like the nuclear approach, but if
it works we'd at least clean up a lot of this mess.
P.
>
>
> >
> >
> > Or we might add locking to it, but IDK what was achieved with RCU
> > here.
> > In any case it's definitely bad that Nouveau has so many redundant
> > and
> > half-redundant mechanisms.
> >
>
> Yeah, agree messing with the locks even more won't help us here.
>
> Regards,
> Christian.
>
>
> >
> >
> >
> > P.
> >
> >
> > >
> > >
> > > P.
> > >
> > >
> > > >
> > > > Regards,
> > > > Christian.
> > > >
> > > >
> > > > >
> > > > > P.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Christian.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Replace the call to dma_fence_is_signaled() with
> > > > > > > nouveau_fence_base_is_signaled().
> > > > > > >
> > > > > > > Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not
> > > > > > > to
> > > > > > > be
> > > > > > > determined
> > > > > > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c
> > > > > > > b/drivers/gpu/drm/nouveau/nouveau_fence.c
> > > > > > > index 7cc84472cece..33535987d8ed 100644
> > > > > > > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c
> > > > > > > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
> > > > > > > @@ -274,7 +274,7 @@ nouveau_fence_done(struct
> > > > > > > nouveau_fence
> > > > > > > *fence)
> > > > > > > nvif_event_block(&fctx->event);
> > > > > > > spin_unlock_irqrestore(&fctx->lock,
> > > > > > > flags);
> > > > > > > }
> > > > > > > - return dma_fence_is_signaled(&fence->base);
> > > > > > > + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > > > > > > &fence-
> > > > > > >
> > > > > > > >
> > > > > > > > base.flags);
> > > > > > > >
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > static long
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
The function brcmf_usb_dl_writeimage() calls the function
brcmf_usb_dl_cmd() but dose not check its return value. The
'state.state' and the 'state.bytes' are uninitialized if the
function brcmf_usb_dl_cmd() fails. It is dangerous to use
uninitialized variables in the conditions.
Add error handling for brcmf_usb_dl_cmd() to jump to error
handling path if the brcmf_usb_dl_cmd() fails and the
'state.state' and the 'state.bytes' are uninitialized.
Improve the error message to report more detailed error
information.
Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 chipsets")
Cc: stable(a)vger.kernel.org # v3.4+
Signed-off-by: Wentao Liang <vulab(a)iscas.ac.cn>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 2821c27f317e..d06c724f63d9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -896,14 +896,16 @@ brcmf_usb_dl_writeimage(struct brcmf_usbdev_info *devinfo, u8 *fw, int fwlen)
}
/* 1) Prepare USB boot loader for runtime image */
- brcmf_usb_dl_cmd(devinfo, DL_START, &state, sizeof(state));
+ err = brcmf_usb_dl_cmd(devinfo, DL_START, &state, sizeof(state));
+ if (err)
+ goto fail;
rdlstate = le32_to_cpu(state.state);
rdlbytes = le32_to_cpu(state.bytes);
/* 2) Check we are in the Waiting state */
if (rdlstate != DL_WAITING) {
- brcmf_err("Failed to DL_START\n");
+ brcmf_err("Invalid DL state: %u\n", rdlstate);
err = -EINVAL;
goto fail;
}
--
2.42.0.windows.2