Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
Hi, On Mon, Feb 28, 2022 at 10:36:59PM +, Limonciello, Mario wrote: > [AMD Official Use Only] > > > -Original Message- > > From: Lukas Wunner > > Sent: Monday, February 28, 2022 16:33 > > To: Bjorn Helgaas > > Cc: Limonciello, Mario ; Mika Westerberg > > ; Michael Jamet > > ; open list:PCI SUBSYSTEM > p...@vger.kernel.org>; open list:THUNDERBOLT DRIVER > u...@vger.kernel.org>; Yehezkel Bernat ; open > > list:DRM DRIVERS ; open list:X86 > > PLATFORM DRIVERS ; Andreas > > Noever ; open list:RADEON and AMDGPU > > DRM DRIVERS ; open list:DRM DRIVER FOR > > NVIDIA GEFORCE/QUADRO GPUS ; Bjorn > > Helgaas ; Deucher, Alexander > > > > Subject: Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from > > PCI > > core > > > > On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote: > > > On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > > > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > > > > facing" > > > > > > assumption above. Not having a Thunderbolt spec, I have no idea > > how > > > > > > you deal with that. > > > > > > > > > > You can download the spec here: > > [...] > > > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > > > > Version 1.0.pdf". > > > > > > > > The spec has Host_Router_indication (bits 18-19) as meaning external > > facing. > > > > I'll respin the patch 3 for using that. > > > > > > Thanks, please include the spec citation when you do. And probably > > > the URL, because it's not at all obvious how the casual reader would > > > get from "is_thunderbolt" to a recent add-on to the USB4 spec. > > > > PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec, > > hence there's no connection between "is_thunderbolt" and the USB4 spec. > > > > It's a proprietary VSEC used by Intel and the only way to recognize > > pre-USB4 Thunderbolt devices that I know of. Its ID is also > > different from the DVSEC IDs given in the above-mentioned spec. > > > > Thanks, > > The USB4 DVSEC spec makes comments about DVSEC_ID of 0x8086 and also > DVSEC VENDOR_ID of 0x8086. Is that not also present on the Intel TBT3 > controllers? > > My interpretation of this (and Mika's comment) was that rather than > looking at the Intel VSEC we should look at the USB4 DVSEC to detect > the Intel TBT3 controllers. For pre-USB4 controllers (TBT 1-3) we need to use the existing method (or a quirk based on device ID) as they don't have the USB4 DVSEC.
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:14:44PM -0800, Linus Torvalds wrote: > On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds > wrote: > > > > We can do > > > > typeof(pos) pos > > > > in the 'for ()' loop, and never use __iter at all. > > > > That means that inside the for-loop, we use a _different_ 'pos' than > > outside. > > The thing that makes me throw up in my mouth a bit is that in that > > typeof(pos) pos > > the first 'pos' (that we use for just the typeof) is that outer-level > 'pos', IOW it's a *different* 'pos' than the second 'pos' in that same > declaration that declares the inner level shadowing new 'pos' > variable. The new "pos" has not yet been declared, so this has to refer to the outer "pos", it cannot be the inner one. Because it hasn't been declared yet :-) Compare this to typeof (pos) pos = pos; where that last "pos" *does* refer to the newly declared one: that declaration has already been done! (So this code is UB btw, 6.3.2.1/2). > If I was a compiler person, I would say "Linus, that thing is too ugly > to live", and I would hate it. I'm just hoping that even compiler > people say "that's *so* ugly it's almost beautiful". It is perfectly well-defined. Well, it would be good if we (GCC) would document it does work, and if someone tested it on LLVM as well. But it is really hard to implement it to *not* work :-) > Because it does seem to work. It's not pretty, but hey, it's not like > our headers are really ever be winning any beauty contests... It is very pretty! Needs a comment though :-) Segher
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 3:45 PM James Bottomley
wrote:
> ...
> > Just from skimming over the patches to change this and experience
> > with the drivers/subsystems I help to maintain I think the primary
> > pattern looks something like this:
> >
> > list_for_each_entry(entry, head, member) {
> > if (some_condition_checking(entry))
> > break;
> > }
> > do_something_with(entry);
>
>
> Actually, we usually have a check to see if the loop found anything,
> but in that case it should something like
>
> if (list_entry_is_head(entry, head, member)) {
> return with error;
> }
> do_somethin_with(entry);
Borrowing from c++, perhaps an explicit end should be used:
if (list_entry_not_end(entry))
do_somethin_with(entry)
It is modelled after c++ and the 'while(begin != end) {}' pattern.
Jeff
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox wrote:
>
> Then we can never use -Wshadow ;-( I'd love to be able to turn it on;
> it catches real bugs.
Oh, we already can never use -Wshadow regardless of things like this.
That bridge hasn't just been burned, it never existed in the first
place.
The whole '-Wshadow' thing simply cannot work with local variables in
macros - something that we've used since day 1.
Try this (as a "p.c" file):
#define min(a,b) ({ \
typeof(a) __a = (a);\
typeof(b) __b = (b);\
__a < __b ? __a : __b; })
int min3(int a, int b, int c)
{
return min(a,min(b,c));
}
and now do "gcc -O2 -S t.c".
Then try it with -Wshadow.
In other words, -Wshadow is simply not acceptable. Never has been,
never will be, and that has nothing to do with the
typeof(pos) pos
kind of thing.
Your argument just isn't an argument.
Linus
[Nouveau] [PATCH 0/6] Remove usage of list iterator past the loop body
This is the first patch removing several categories of use cases of
the list iterator variable past the loop.
This is follow up to the discussion in:
https://lore.kernel.org/all/20220217184829.1991035-1-jakobkosc...@gmail.com/
As concluded in:
https://lore.kernel.org/all/yhdfeiwi4edth...@kroah.com/
the correct use should be using a separate variable after the loop
and using a 'tmp' variable as the list iterator.
The list iterator will not point to a valid structure after the loop
if no break/goto was hit. Invalid uses of the list iterator variable
can be avoided altogether by simply using a separate pointer to
iterate the list.
Linus and Greg agreed on the following pattern:
- struct gr_request *req;
+ struct gr_request *req = NULL;
+ struct gr_request *tmp;
struct gr_ep *ep;
int ret = 0;
- list_for_each_entry(req, >queue, queue) {
- if (>req == _req)
+ list_for_each_entry(tmp, >queue, queue) {
+ if (>req == _req) {
+ req = tmp;
break;
+ }
}
- if (>req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
With gnu89 the list iterator variable cannot yet be declared
within the for loop of the list iterator.
Moving to a more modern version of C would allow defining
the list iterator variable within the macro, limiting
the scope to the loop.
This avoids any incorrect usage past the loop altogether.
This are around 30% of the cases where the iterator
variable is used past the loop (identified with a slightly
modified version of use_after_iter.cocci).
I've decided to split it into at a few patches separated
by similar use cases.
Because the output of get_maintainer.pl was too big,
I included all the found lists and everyone from the
previous discussion.
Jakob Koschel (6):
drivers: usb: remove usage of list iterator past the loop body
treewide: remove using list iterator after loop body as a ptr
treewide: fix incorrect use to determine if list is empty
drivers: remove unnecessary use of list iterator variable
treewide: remove dereference of list iterator after loop body
treewide: remove check of list iterator against head past the loop
body
arch/arm/mach-mmp/sram.c | 9 ++--
arch/arm/plat-pxa/ssp.c | 28 +---
arch/powerpc/sysdev/fsl_gtm.c | 4 +-
arch/x86/kernel/cpu/sgx/encl.c| 6 ++-
drivers/block/drbd/drbd_req.c | 45 ---
drivers/counter/counter-chrdev.c | 26 ++-
drivers/crypto/cavium/nitrox/nitrox_main.c| 11 +++--
drivers/dma/dw-edma/dw-edma-core.c| 4 +-
drivers/dma/ppc4xx/adma.c | 11 +++--
drivers/firewire/core-transaction.c | 32 +++--
drivers/firewire/sbp2.c | 14 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 19 +---
drivers/gpu/drm/drm_memory.c | 15 ---
drivers/gpu/drm/drm_mm.c | 17 ---
drivers/gpu/drm/drm_vm.c | 13 +++---
drivers/gpu/drm/gma500/oaktrail_lvds.c| 9 ++--
drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 ---
drivers/gpu/drm/i915/gt/intel_ring.c | 15 ---
.../gpu/drm/nouveau/nvkm/subdev/clk/base.c| 11 +++--
.../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++---
drivers/gpu/drm/scheduler/sched_main.c| 14 +++---
drivers/gpu/drm/ttm/ttm_bo.c | 19
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +
drivers/infiniband/hw/hfi1/tid_rdma.c | 16 ---
drivers/infiniband/hw/mlx4/main.c | 12 ++---
drivers/media/dvb-frontends/mxl5xx.c | 11 +++--
drivers/media/pci/saa7134/saa7134-alsa.c | 4 +-
drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++--
drivers/misc/mei/interrupt.c | 12 ++---
.../net/ethernet/intel/i40e/i40e_ethtool.c| 3 +-
.../net/ethernet/qlogic/qede/qede_filter.c| 11 +++--
drivers/net/wireless/ath/ath6kl/htc_mbox.c| 2 +-
.../net/wireless/intel/ipw2x00/libipw_rx.c| 15 ---
drivers/perf/xgene_pmu.c | 13 +++---
drivers/power/supply/cpcap-battery.c | 11 +++--
drivers/scsi/lpfc/lpfc_bsg.c | 16 ---
drivers/scsi/scsi_transport_sas.c | 17 ---
drivers/scsi/wd719x.c | 12 +++--
drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++
drivers/staging/rtl8192e/rtllib_rx.c | 17 ---
.../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 ---
.../rtl8192u/ieee80211/rtl819x_TSProc.c | 19
drivers/thermal/thermal_core.c| 38 ++--
drivers/usb/gadget/composite.c| 9 ++--
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel wrote: > > The goal of this is to get compiler warnings right? This would indeed be > great. Yes, so I don't mind having a one-time patch that has been gathered using some automated checker tool, but I don't think that works from a long-term maintenance perspective. So if we have the basic rule being "don't use the loop iterator after the loop has finished, because it can cause all kinds of subtle issues", then in _addition_ to fixing the existing code paths that have this issue, I really would want to (a) get a compiler warning for future cases and (b) make it not actually _work_ for future cases. Because otherwise it will just happen again. > Changing the list_for_each_entry() macro first will break all of those cases > (e.g. the ones using 'list_entry_is_head()). So I have no problems with breaking cases that we basically already have a patch for due to your automated tool. There were certainly more than a handful, but it didn't look _too_ bad to just make the rule be "don't use the iterator after the loop". Of course, that's just based on that patch of yours. Maybe there are a ton of other cases that your patch didn't change, because they didn't match your trigger case, so I may just be overly optimistic here. But basically to _me_, the important part is that the end result is maintainable longer-term. I'm more than happy to have a one-time patch to fix a lot of dubious cases if we can then have clean rules going forward. > I assumed it is better to fix those cases first and then have a simple > coccinelle script changing the macro + moving the iterator into the scope > of the macro. So that had been another plan of mine, until I actually looked at changing the macro. In the one case I looked at, it was ugly beyond belief. It turns out that just syntactically, it's really nice to give the type of the iterator from outside the way we do now. Yeah, it may be a bit odd, and maybe it's partly because I'm so used to the "list_for_each_list_entry()" syntax, but moving the type into the loop construct really made it nasty - either one very complex line, or having to split it over two lines which was even worse. Maybe the place I looked at just happened to have a long typename, but it's basically always going to be a struct, so it's never a _simple_ type. And it just looked very odd adn unnatural to have the type as one of the "arguments" to that list_for_each_entry() macro. So yes, initially my idea had been to just move the iterator entirely inside the macro. But specifying the type got so ugly that I think that typeof (pos) pos trick inside the macro really ends up giving us the best of all worlds: (a) let's us keep the existing syntax and code for all the nice cases that did everything inside the loop anyway (b) gives us a nice warning for any normal use-after-loop case (unless you explicitly initialized it like that sgx_mmu_notifier_release() function did for no good reason (c) also guarantees that even if you don't get a warning, non-converted (or newly written) bad code won't actually _work_ so you end up getting the new rules without any ambiguity or mistaken > With this you are no longer able to set the 'outer' pos within the list > iterator loop body or am I missing something? Correct. Any assignment inside the loop will be entirely just to the local loop case. So any "break;" out of the loop will have to set another variable - like your updated patch did. > I fail to see how this will make most of the changes in this > patch obsolete (if that was the intention). I hope my explanation above clarifies my thinking: I do not dislike your patch, and in fact your patch is indeed required to make the new semantics work. What I disliked was always the maintainability of your patch - making the rules be something that isn't actually visible in the source code, and letting the old semantics still work as well as they ever did, and having to basically run some verification pass to find bad users. (I also disliked your original patch that mixed up the "CPU speculation type safety" with the actual non-speculative problems, but that was another issue). Linus
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 12:20, Greg KH wrote: > > On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: >> If the list does not contain the expected element, the value of >> list_for_each_entry() iterator will not point to a valid structure. >> To avoid type confusion in such case, the list iterator >> scope will be limited to list_for_each_entry() loop. >> >> In preparation to limiting scope of a list iterator to the list traversal >> loop, use a dedicated pointer to point to the found element. >> Determining if an element was found is then simply checking if >> the pointer is != NULL. >> >> Signed-off-by: Jakob Koschel >> --- >> arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- >> drivers/scsi/scsi_transport_sas.c| 17 - >> drivers/thermal/thermal_core.c | 38 ++-- >> drivers/usb/gadget/configfs.c| 22 ++-- >> drivers/usb/gadget/udc/max3420_udc.c | 11 +--- >> drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- >> drivers/usb/mtu3/mtu3_gadget.c | 11 +--- >> drivers/usb/musb/musb_gadget.c | 11 +--- >> drivers/vfio/mdev/mdev_core.c| 11 +--- >> 9 files changed, 88 insertions(+), 50 deletions(-) > > The drivers/usb/ portion of this patch should be in patch 1/X, right? I kept them separate since it's a slightly different case. Using 'ptr' instead of '>other_member'. Regardless, I'll split this commit up as you mentioned. > > Also, you will have to split these up per-subsystem so that the > different subsystem maintainers can take these in their trees. Thanks for the feedback. To clarify I understand you correctly: I should do one patch set per-subsystem with every instance/(file?) fixed as a separate commit? If I understand correctly, I'll repost accordingly. > > thanks, > > greg k-h thanks, Jakob Koschel
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 3:26 PM Matthew Wilcox wrote:
>
> #define ___PASTE(a, b) a##b
> #define __PASTE(a, b) ___PASTE(a, b)
> #define _min(a, b, u) ({ \
Yeah, except that's ugly beyond belief, plus it's literally not what
we do in the kernel.
Really. The "-Wshadow doesn't work on the kernel" is not some new
issue, because you have to do completely insane things to the source
code to enable it.
Just compare your uglier-than-sin version to my straightforward one.
One does the usual and obvious "use a private variable to avoid the
classic multi-use of a macro argument". And the other one is an
abomination.
Linus
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/scsi/scsi_transport_sas.c
> b/drivers/scsi/scsi_transport_sas.c
> index 4ee578b181da..a8cbd90db9d2 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy);
> * connected to a remote device is a port, so ports must be formed on
> * all devices with phys if they're connected to anything.
> */
> -void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
> +void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy)
_phy is an unfortunate name.
> {
> mutex_lock(>phy_list_mutex);
> - if (unlikely(!list_empty(>port_siblings))) {
> + if (unlikely(!list_empty(&_phy->port_siblings))) {
> /* make sure we're already on this port */
> + struct sas_phy *phy = NULL;
Maybe call this port_phy?
> struct sas_phy *tmp;
>
> list_for_each_entry(tmp, >phy_list, port_siblings)
> - if (tmp == phy)
> + if (tmp == _phy) {
> + phy = tmp;
> break;
> + }
> /* If this trips, you added a phy that was already
>* part of a different port */
> - if (unlikely(tmp != phy)) {
> + if (unlikely(!phy)) {
> dev_printk(KERN_ERR, >dev, "trying to add phy %s
> fails: it's already part of another port\n",
> -dev_name(>dev));
> +dev_name(&_phy->dev));
> BUG();
> }
> } else {
> - sas_port_create_link(port, phy);
> - list_add_tail(>port_siblings, >phy_list);
> + sas_port_create_link(port, _phy);
> + list_add_tail(&_phy->port_siblings, >phy_list);
> port->num_phys++;
> }
> mutex_unlock(>phy_list_mutex);
regards,
dan carpenter
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: > If the list does not contain the expected element, the value of > list_for_each_entry() iterator will not point to a valid structure. > To avoid type confusion in such case, the list iterator > scope will be limited to list_for_each_entry() loop. > > In preparation to limiting scope of a list iterator to the list traversal > loop, use a dedicated pointer to point to the found element. > Determining if an element was found is then simply checking if > the pointer is != NULL. > > Signed-off-by: Jakob Koschel > --- > arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- > drivers/scsi/scsi_transport_sas.c| 17 - > drivers/thermal/thermal_core.c | 38 ++-- > drivers/usb/gadget/configfs.c| 22 ++-- > drivers/usb/gadget/udc/max3420_udc.c | 11 +--- > drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- > drivers/usb/mtu3/mtu3_gadget.c | 11 +--- > drivers/usb/musb/musb_gadget.c | 11 +--- > drivers/vfio/mdev/mdev_core.c| 11 +--- > 9 files changed, 88 insertions(+), 50 deletions(-) The drivers/usb/ portion of this patch should be in patch 1/X, right? Also, you will have to split these up per-subsystem so that the different subsystem maintainers can take these in their trees. thanks, greg k-h
Re: [Nouveau] [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
This is a bit more work (and a lot more noise), but I'd prefer if this were split into as many patches as there are components. I'm not going to review the parts of the patches that don't concern me, and if something turns out to be a problem later one (it shouldn't but one never knows) it'll be much easier to revert or put the blame on an individual smaller commit than on this... With that being said, ultimately I don't care that much and will leave that to people who do :) Jakob Koschel wrote on Mon, Feb 28, 2022 at 12:08:22PM +0100: > net/9p/trans_xen.c| 11 +++-- This 9p change looks good to me. Reviewed-by: Dominique Martinet # 9p -- Dominique
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
> > On Mon, Feb 28, 2022 at 4:19 AM Christian König
> > wrote:
> > > I don't think that using the extra variable makes the code in any
> > > way
> > > more reliable or easier to read.
> > So I think the next step is to do the attached patch (which
> > requires
> > that "-std=gnu11" that was discussed in the original thread).
> >
> > That will guarantee that the 'pos' parameter of
> > list_for_each_entry()
> > is only updated INSIDE the for_each_list_entry() loop, and can
> > never
> > point to the (wrongly typed) head entry.
> >
> > And I would actually hope that it should actually cause compiler
> > warnings about possibly uninitialized variables if people then use
> > the
> > 'pos' pointer outside the loop. Except
> >
> > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL
> > for
> > inexplicable reasons - possibly because it already expected this
> > behavior
> >
> > (b) when I remove that NULL initializer, I still don't get a
> > warning,
> > because we've disabled -Wno-maybe-uninitialized since it results in
> > so
> > many false positives.
> >
> > Oh well.
> >
> > Anyway, give this patch a look, and at least if it's expanded to do
> > "(pos) = NULL" in the entry statement for the for-loop, it will
> > avoid the HEAD type confusion that Jakob is working on. And I think
> > in a cleaner way than the horrid games he plays.
> >
> > (But it won't avoid possible CPU speculation of such type
> > confusion. That, in my opinion, is a completely different issue)
>
> Yes, completely agree.
>
> > I do wish we could actually poison the 'pos' value after the loop
> > somehow - but clearly the "might be uninitialized" I was hoping for
> > isn't the way to do it.
> >
> > Anybody have any ideas?
>
> I think we should look at the use cases why code is touching (pos)
> after the loop.
>
> Just from skimming over the patches to change this and experience
> with the drivers/subsystems I help to maintain I think the primary
> pattern looks something like this:
>
> list_for_each_entry(entry, head, member) {
> if (some_condition_checking(entry))
> break;
> }
> do_something_with(entry);
Actually, we usually have a check to see if the loop found anything,
but in that case it should something like
if (list_entry_is_head(entry, head, member)) {
return with error;
}
do_somethin_with(entry);
Suffice? The list_entry_is_head() macro is designed to cope with the
bogus entry on head problem.
James
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 23:59 +0200, Mike Rapoport wrote:
>
> On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley <
> james.bottom...@hansenpartnership.com> wrote:
> > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
[...]
> > > > I do wish we could actually poison the 'pos' value after the
> > > > loop somehow - but clearly the "might be uninitialized" I was
> > > > hoping for isn't the way to do it.
> > > >
> > > > Anybody have any ideas?
> > >
> > > I think we should look at the use cases why code is touching
> > > (pos) after the loop.
> > >
> > > Just from skimming over the patches to change this and experience
> > > with the drivers/subsystems I help to maintain I think the
> > > primary pattern looks something like this:
> > >
> > > list_for_each_entry(entry, head, member) {
> > > if (some_condition_checking(entry))
> > > break;
> > > }
> > > do_something_with(entry);
> >
> > Actually, we usually have a check to see if the loop found
> > anything, but in that case it should something like
> >
> > if (list_entry_is_head(entry, head, member)) {
> >return with error;
> > }
> > do_somethin_with(entry);
> >
> > Suffice? The list_entry_is_head() macro is designed to cope with
> > the bogus entry on head problem.
>
> Won't suffice because the end goal of this work is to limit scope of
> entry only to loop. Hence the need for additional variable.
Well, yes, but my objection is more to the size of churn than the
desire to do loop local. I'm not even sure loop local is possible,
because it's always annoyed me that for (int i = 0; ... in C++ defines
i in the outer scope not the loop scope, which is why I never use it.
However, if the desire is really to poison the loop variable then we
can do
#define list_for_each_entry(pos, head, member) \
for (pos = list_first_entry(head, typeof(*pos), member);\
!list_entry_is_head(pos, head, member) && ((pos = NULL) == NULL;
\
pos = list_next_entry(pos, member))
Which would at least set pos to NULL when the loop completes.
> Besides, there are no guarantees that people won't
> do_something_with(entry) without the check or won't compare entry to
> NULL to check if the loop finished with break or not.
I get the wider goal, but we have to patch the problem cases now and a
simple one-liner is better than a larger patch that may or may not work
if we ever achieve the local definition or value poisoning idea. I'm
also fairly certain coccinelle can come up with a use without checking
for loop completion semantic patch which we can add to 0day.
James
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
From: Matthew Wilcox > Sent: 28 February 2022 20:16 > > On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote: > > We can do > > > > typeof(pos) pos > > > > in the 'for ()' loop, and never use __iter at all. > > > > That means that inside the for-loop, we use a _different_ 'pos' than > > outside. > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > it catches real bugs. > > > +#define list_for_each_entry(pos, head, member) > > \ > > + for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); > > \ > > +!list_entry_is_head(pos, head, member);\ > > pos = list_next_entry(pos, member)) Actually can't you use 'pos' to temporarily hold the address of 'member'. Something like: for (pos = (void *)head; \ pos ? ((pos = (void *)pos - offsetof(member)), 1) : 0; \ pos = (void *)pos->next) So that 'pos' is NULL if the loop terminates. No pointers outside structures are generated. Probably need to kill list_entry_is_head() - or it just checks for NULL. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 05:28:58PM -0500, James Bottomley wrote:
> On Mon, 2022-02-28 at 23:59 +0200, Mike Rapoport wrote:
> >
> > On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley <
> > james.bottom...@hansenpartnership.com> wrote:
> > > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
> [...]
> > > > > I do wish we could actually poison the 'pos' value after the
> > > > > loop somehow - but clearly the "might be uninitialized" I was
> > > > > hoping for isn't the way to do it.
> > > > >
> > > > > Anybody have any ideas?
> > > >
> > > > I think we should look at the use cases why code is touching
> > > > (pos) after the loop.
> > > >
> > > > Just from skimming over the patches to change this and experience
> > > > with the drivers/subsystems I help to maintain I think the
> > > > primary pattern looks something like this:
> > > >
> > > > list_for_each_entry(entry, head, member) {
> > > > if (some_condition_checking(entry))
> > > > break;
> > > > }
> > > > do_something_with(entry);
> > >
> > > Actually, we usually have a check to see if the loop found
> > > anything, but in that case it should something like
> > >
> > > if (list_entry_is_head(entry, head, member)) {
> > >return with error;
> > > }
> > > do_somethin_with(entry);
> > >
> > > Suffice? The list_entry_is_head() macro is designed to cope with
> > > the bogus entry on head problem.
> >
> > Won't suffice because the end goal of this work is to limit scope of
> > entry only to loop. Hence the need for additional variable.
>
> Well, yes, but my objection is more to the size of churn than the
> desire to do loop local. I'm not even sure loop local is possible,
> because it's always annoyed me that for (int i = 0; ... in C++ defines
> i in the outer scope not the loop scope, which is why I never use it.
In C its scope is the rest of the declaration and the entire loop, not
anything after it. This was the same in C++98 already, btw (but in
pre-standard versions of C++ things were like you remember, yes, and it
was painful).
Segher
Re: [Nouveau] (subset) [PATCH v2 13/22] drm/nouveau/kms: Remove redundant zpos initialisation
On Mon, 21 Feb 2022 10:59:09 +0100, Maxime Ripard wrote: > The nouveau KMS driver will call drm_plane_create_zpos_property() with > an init value depending on the plane purpose. > > Since the initial value wasn't carried over in the state, the driver had > to set it again in nv50_wndw_reset(). However, the helpers have been > adjusted to set it properly at reset, so this is not needed anymore. > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 21:10, Linus Torvalds > wrote: > > On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds > wrote: >> >> Side note: we do need *some* way to do it. > > Ooh. > > This patch is a work of art. > > And I mean that in the worst possible way. > > We can do > >typeof(pos) pos > > in the 'for ()' loop, and never use __iter at all. > > That means that inside the for-loop, we use a _different_ 'pos' than outside. > > And then the compiler will not see some "might be uninitialized", but > the outer 'pos' *will* be uninitialized. > > Unless, of course, the outer 'pos' had that pointless explicit initializer. The goal of this is to get compiler warnings right? This would indeed be great. Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()). I assumed it is better to fix those cases first and then have a simple coccinelle script changing the macro + moving the iterator into the scope of the macro. > > Here - can somebody poke holes in this "work of art" patch? With this you are no longer able to set the 'outer' pos within the list iterator loop body or am I missing something? Like this it stays uninitialized but you'll probably want to set it from within the loop. You would then yet again need a variable with another name to use after the loop. I fail to see how this will make most of the changes in this patch obsolete (if that was the intention). > > Linus > - Jakob
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley
wrote:
>On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
>> > On Mon, Feb 28, 2022 at 4:19 AM Christian König
>> > wrote:
>> > > I don't think that using the extra variable makes the code in any
>> > > way
>> > > more reliable or easier to read.
>> > So I think the next step is to do the attached patch (which
>> > requires
>> > that "-std=gnu11" that was discussed in the original thread).
>> >
>> > That will guarantee that the 'pos' parameter of
>> > list_for_each_entry()
>> > is only updated INSIDE the for_each_list_entry() loop, and can
>> > never
>> > point to the (wrongly typed) head entry.
>> >
>> > And I would actually hope that it should actually cause compiler
>> > warnings about possibly uninitialized variables if people then use
>> > the
>> > 'pos' pointer outside the loop. Except
>> >
>> > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL
>> > for
>> > inexplicable reasons - possibly because it already expected this
>> > behavior
>> >
>> > (b) when I remove that NULL initializer, I still don't get a
>> > warning,
>> > because we've disabled -Wno-maybe-uninitialized since it results in
>> > so
>> > many false positives.
>> >
>> > Oh well.
>> >
>> > Anyway, give this patch a look, and at least if it's expanded to do
>> > "(pos) = NULL" in the entry statement for the for-loop, it will
>> > avoid the HEAD type confusion that Jakob is working on. And I think
>> > in a cleaner way than the horrid games he plays.
>> >
>> > (But it won't avoid possible CPU speculation of such type
>> > confusion. That, in my opinion, is a completely different issue)
>>
>> Yes, completely agree.
>>
>> > I do wish we could actually poison the 'pos' value after the loop
>> > somehow - but clearly the "might be uninitialized" I was hoping for
>> > isn't the way to do it.
>> >
>> > Anybody have any ideas?
>>
>> I think we should look at the use cases why code is touching (pos)
>> after the loop.
>>
>> Just from skimming over the patches to change this and experience
>> with the drivers/subsystems I help to maintain I think the primary
>> pattern looks something like this:
>>
>> list_for_each_entry(entry, head, member) {
>> if (some_condition_checking(entry))
>> break;
>> }
>> do_something_with(entry);
>
>
>Actually, we usually have a check to see if the loop found anything,
>but in that case it should something like
>
>if (list_entry_is_head(entry, head, member)) {
>return with error;
>}
>do_somethin_with(entry);
>
>Suffice? The list_entry_is_head() macro is designed to cope with the
>bogus entry on head problem.
Won't suffice because the end goal of this work is to limit scope of entry only
to loop. Hence the need for additional variable.
Besides, there are no guarantees that people won't do_something_with(entry)
without the check or won't compare entry to NULL to check if the loop finished
with break or not.
>James
--
Sincerely yours,
Mike
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 20:16 +, Matthew Wilcox wrote: > On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote: > > We can do > > > > typeof(pos) pos > > > > in the 'for ()' loop, and never use __iter at all. > > > > That means that inside the for-loop, we use a _different_ 'pos' than > > outside. > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > it catches real bugs. > I was just going to say the same thing... If we're willing to change the API for the macro, we could do list_for_each_entry(type, pos, head, member) and then actually take advantage of -Wshadow? johannes
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:37:15PM -0800, Linus Torvalds wrote:
> On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox wrote:
> >
> > Then we can never use -Wshadow ;-( I'd love to be able to turn it on;
> > it catches real bugs.
>
> Oh, we already can never use -Wshadow regardless of things like this.
> That bridge hasn't just been burned, it never existed in the first
> place.
>
> The whole '-Wshadow' thing simply cannot work with local variables in
> macros - something that we've used since day 1.
>
> Try this (as a "p.c" file):
>
> #define min(a,b) ({ \
> typeof(a) __a = (a);\
> typeof(b) __b = (b);\
> __a < __b ? __a : __b; })
>
> int min3(int a, int b, int c)
> {
> return min(a,min(b,c));
> }
>
> and now do "gcc -O2 -S t.c".
>
> Then try it with -Wshadow.
#define ___PASTE(a, b) a##b
#define __PASTE(a, b) ___PASTE(a, b)
#define _min(a, b, u) ({ \
typeof(a) __PASTE(__a,u) = (a);\
typeof(b) __PASTE(__b,u) = (b);\
__PASTE(__a,u) < __PASTE(__b,u) ? __PASTE(__a,u) : __PASTE(__b,u); })
#define min(a, b) _min(a, b, __COUNTER__)
int min3(int a, int b, int c)
{
return min(a,min(b,c));
}
(probably there's a more elegant way to do this)
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:45 PM Linus Torvalds wrote: > > Yeah, except that's ugly beyond belief, plus it's literally not what > we do in the kernel. (Of course, I probably shouldn't have used 'min()' as an example, because that is actually one of the few places where we do exactly that, using our __UNIQUE_ID() macros. Exactly because people _have_ tried to do -Wshadow when doing W=2). Linus
Re: [Nouveau] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote: > a multi-line indent gets curly braces for readability even though > it's not required by C. And then both sides would get curly braces. That's more your personal preference than a coding style guideline.
Re: [Nouveau] [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
On Mon, Feb 28, 2022 at 12:08:22PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c
> b/drivers/infiniband/hw/hfi1/tid_rdma.c
> index 2a7abf7a1f7f..a069847b56aa 100644
> --- a/drivers/infiniband/hw/hfi1/tid_rdma.c
> +++ b/drivers/infiniband/hw/hfi1/tid_rdma.c
> @@ -1239,7 +1239,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> struct hfi1_ctxtdata *rcd = flow->req->rcd;
> struct hfi1_devdata *dd = rcd->dd;
> u32 ngroups, pageidx = 0;
> - struct tid_group *group = NULL, *used;
> + struct tid_group *group = NULL, *used, *tmp;
> u8 use;
>
> flow->tnode_cnt = 0;
> @@ -1248,13 +1248,15 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> goto used_list;
>
> /* First look at complete groups */
> - list_for_each_entry(group, >tid_group_list.list, list) {
> - kern_add_tid_node(flow, rcd, "complete groups", group,
> - group->size);
> + list_for_each_entry(tmp, >tid_group_list.list, list) {
> + kern_add_tid_node(flow, rcd, "complete groups", tmp,
> + tmp->size);
>
> - pageidx += group->size;
> - if (!--ngroups)
> + pageidx += tmp->size;
> + if (!--ngroups) {
> + group = tmp;
> break;
> + }
> }
>
> if (pageidx >= flow->npagesets)
> @@ -1277,7 +1279,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>* However, if we are at the head, we have reached the end of the
^
>* complete groups list from the first loop above
^^
>*/
Originally this code tested for an open code list_is_head() so the
comment made sense, but it's out of date now. Just delete it.
> - if (group && >list == >tid_group_list.list)
> + if (!group)
> goto bail_eagain;
> group = list_prepare_entry(group, >tid_group_list.list,
> list);
regards,
dan carpenter
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:19 AM Christian König wrote: > > I don't think that using the extra variable makes the code in any way > more reliable or easier to read. So I think the next step is to do the attached patch (which requires that "-std=gnu11" that was discussed in the original thread). That will guarantee that the 'pos' parameter of list_for_each_entry() is only updated INSIDE the for_each_list_entry() loop, and can never point to the (wrongly typed) head entry. And I would actually hope that it should actually cause compiler warnings about possibly uninitialized variables if people then use the 'pos' pointer outside the loop. Except (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for inexplicable reasons - possibly because it already expected this behavior (b) when I remove that NULL initializer, I still don't get a warning, because we've disabled -Wno-maybe-uninitialized since it results in so many false positives. Oh well. Anyway, give this patch a look, and at least if it's expanded to do "(pos) = NULL" in the entry statement for the for-loop, it will avoid the HEAD type confusion that Jakob is working on. And I think in a cleaner way than the horrid games he plays. (But it won't avoid possible CPU speculation of such type confusion. That, in my opinion, is a completely different issue) I do wish we could actually poison the 'pos' value after the loop somehow - but clearly the "might be uninitialized" I was hoping for isn't the way to do it. Anybody have any ideas? Linus p Description: Binary data
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds
wrote:
>
> Side note: we do need *some* way to do it.
Ooh.
This patch is a work of art.
And I mean that in the worst possible way.
We can do
typeof(pos) pos
in the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
And then the compiler will not see some "might be uninitialized", but
the outer 'pos' *will* be uninitialized.
Unless, of course, the outer 'pos' had that pointless explicit initializer.
Here - can somebody poke holes in this "work of art" patch?
Linus
Makefile | 2 +-
arch/x86/kernel/cpu/sgx/encl.c | 2 +-
include/linux/list.h | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index daeb5c88b50b..cc4b0a266af0 100644
--- a/Makefile
+++ b/Makefile
@@ -515,7 +515,7 @@ KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
-Werror=implicit-function-declaration -Werror=implicit-int \
-Werror=return-type -Wno-format-security \
- -std=gnu89
+ -std=gnu11
KBUILD_CPPFLAGS := -D__KERNEL__
KBUILD_AFLAGS_KERNEL :=
KBUILD_CFLAGS_KERNEL :=
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 48afe96ae0f0..87db2f3936b0 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -450,7 +450,7 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
struct mm_struct *mm)
{
struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier);
- struct sgx_encl_mm *tmp = NULL;
+ struct sgx_encl_mm *tmp;
/*
* The enclave itself can remove encl_mm. Note, objects can't be moved
diff --git a/include/linux/list.h b/include/linux/list.h
index dd6c2041d09c..708078b2f24d 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -634,9 +634,9 @@ static inline void list_splice_tail_init(struct list_head *list,
* @head: the head for your list.
* @member: the name of the list_head within the struct.
*/
-#define list_for_each_entry(pos, head, member)\
- for (pos = list_first_entry(head, typeof(*pos), member); \
- !list_entry_is_head(pos, head, member); \
+#define list_for_each_entry(pos, head, member) \
+ for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); \
+ !list_entry_is_head(pos, head, member); \
pos = list_next_entry(pos, member))
/**
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:38 PM Segher Boessenkool wrote: > > In C its scope is the rest of the declaration and the entire loop, not > anything after it. This was the same in C++98 already, btw (but in > pre-standard versions of C++ things were like you remember, yes, and it > was painful). Yeah, the original C++ model was just unadulterated garbage, with no excuse for it, and the scope was not the loop, but the block the loop existed in. That would never have been acceptable for the kernel - it's basically just an even uglier version of "put variable declarations in the middle of code" (and we use "-Wdeclaration-after-statement" to disallow that for kernel code, although apparently some of our user space tooling code doesn't enforce or follow that rule). The actual C99 version is the sane one which actually makes it easier and clearer to have loop iterators that are clearly just in loop scope. That's a good idea in general, and I have wanted to start using that in the kernel even aside from some of the loop construct macros. Because putting variables in natural minimal scope is a GoodThing(tm). Of course, we shouldn't go crazy with it. Even after we do that -std=gnu11 thing, we'll have backports to worry about. And it's not clear that we necessarily want to backport that gnu11 thing - since people who run old stable kernels also may be still using those really old compilers... Linus
[Nouveau] [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
>member == head, using the iterator variable after the loop should
be avoided.
In preparation to limiting the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element.
Signed-off-by: Jakob Koschel
---
arch/arm/mach-mmp/sram.c | 9 ++--
arch/arm/plat-pxa/ssp.c | 28 +---
drivers/block/drbd/drbd_req.c | 45 ---
drivers/counter/counter-chrdev.c | 26 ++-
drivers/crypto/cavium/nitrox/nitrox_main.c| 11 +++--
drivers/dma/ppc4xx/adma.c | 11 +++--
drivers/firewire/core-transaction.c | 32 +++--
drivers/firewire/sbp2.c | 14 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 19 +---
drivers/gpu/drm/drm_memory.c | 15 ---
drivers/gpu/drm/drm_mm.c | 17 ---
drivers/gpu/drm/drm_vm.c | 13 +++---
drivers/gpu/drm/gma500/oaktrail_lvds.c| 9 ++--
drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 ---
drivers/gpu/drm/i915/gt/intel_ring.c | 15 ---
.../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++---
drivers/gpu/drm/scheduler/sched_main.c| 14 +++---
drivers/gpu/drm/ttm/ttm_bo.c | 19
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +
drivers/infiniband/hw/hfi1/tid_rdma.c | 16 ---
drivers/infiniband/hw/mlx4/main.c | 12 ++---
drivers/media/dvb-frontends/mxl5xx.c | 11 +++--
drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++--
drivers/misc/mei/interrupt.c | 12 ++---
.../net/ethernet/qlogic/qede/qede_filter.c| 11 +++--
.../net/wireless/intel/ipw2x00/libipw_rx.c| 15 ---
drivers/power/supply/cpcap-battery.c | 11 +++--
drivers/scsi/lpfc/lpfc_bsg.c | 16 ---
drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++
drivers/staging/rtl8192e/rtllib_rx.c | 17 ---
.../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 ---
.../rtl8192u/ieee80211/rtl819x_TSProc.c | 19
drivers/usb/gadget/composite.c| 9 ++--
fs/cifs/smb2misc.c| 10 +++--
fs/proc/kcore.c | 13 +++---
kernel/debug/kdb/kdb_main.c | 36 +--
kernel/power/snapshot.c | 10 +++--
kernel/trace/ftrace.c | 22 +
kernel/trace/trace_eprobe.c | 15 ---
kernel/trace/trace_events.c | 11 ++---
net/9p/trans_xen.c| 11 +++--
net/ipv4/udp_tunnel_nic.c | 10 +++--
net/tipc/name_table.c | 11 +++--
net/tipc/socket.c | 11 +++--
net/xfrm/xfrm_ipcomp.c| 11 +++--
sound/soc/intel/catpt/pcm.c | 13 +++---
sound/soc/sprd/sprd-mcdt.c| 13 +++---
48 files changed, 455 insertions(+), 315 deletions(-)
diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c
index 6794e2db1ad5..fc47c107059b 100644
--- a/arch/arm/mach-mmp/sram.c
+++ b/arch/arm/mach-mmp/sram.c
@@ -39,19 +39,22 @@ static LIST_HEAD(sram_bank_list);
struct gen_pool *sram_get_gpool(char *pool_name)
{
struct sram_bank_info *info = NULL;
+ struct sram_bank_info *tmp;
if (!pool_name)
return NULL;
mutex_lock(_lock);
- list_for_each_entry(info, _bank_list, node)
- if (!strcmp(pool_name, info->pool_name))
+ list_for_each_entry(tmp, _bank_list, node)
+ if (!strcmp(pool_name, tmp->pool_name)) {
+ info = tmp;
break;
+ }
mutex_unlock(_lock);
- if (>node == _bank_list)
+ if (!info)
return NULL;
return info->gpool;
diff --git a/arch/arm/plat-pxa/ssp.c b/arch/arm/plat-pxa/ssp.c
index 563440315acd..4884a8c0c89b 100644
--- a/arch/arm/plat-pxa/ssp.c
+++ b/arch/arm/plat-pxa/ssp.c
@@ -38,22 +38,20 @@ static LIST_HEAD(ssp_list);
struct ssp_device *pxa_ssp_request(int port, const char *label)
{
struct ssp_device *ssp = NULL;
+ struct ssp_device *tmp;
mutex_lock(_lock);
- list_for_each_entry(ssp, _list, node) {
- if (ssp->port_id == port && ssp->use_count == 0) {
- ssp->use_count++;
- ssp->label = label;
+ list_for_each_entry(tmp,
Re: [Nouveau] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
> On 28. Feb 2022, at 12:24, Dan Carpenter wrote:
>
> On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
>> diff --git a/drivers/usb/gadget/udc/at91_udc.c
>> b/drivers/usb/gadget/udc/at91_udc.c
>> index 9040a0561466..0fd0307bc07b 100644
>> --- a/drivers/usb/gadget/udc/at91_udc.c
>> +++ b/drivers/usb/gadget/udc/at91_udc.c
>> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct
>> at91_ep *ep)
>> if (list_empty (>queue))
>> seq_printf(s, "\t(queue empty)\n");
>>
>> -else list_for_each_entry (req, >queue, queue) {
>> -unsignedlength = req->req.actual;
>> +else
>> +list_for_each_entry(req, >queue, queue) {
>> +unsigned intlength = req->req.actual;
>>
>> -seq_printf(s, "\treq %p len %d/%d buf %p\n",
>> ->req, length,
>> -req->req.length, req->req.buf);
>> -}
>> +seq_printf(s, "\treq %p len %d/%d buf %p\n",
>> +>req, length,
>> +req->req.length, req->req.buf);
>> +}
>
> Don't make unrelated white space changes. It just makes the patch
> harder to review. As you're writing the patch make note of any
> additional changes and do them later in a separate patch.
>
> Also a multi-line indent gets curly braces for readability even though
> it's not required by C. And then both sides would get curly braces.
>
>> spin_unlock_irqrestore(>lock, flags);
>> }
>>
>> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void
>> *unused)
>>
>> if (udc->enabled && udc->vbus) {
>> proc_ep_show(s, >ep[0]);
>> -list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
>> +list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
>
> Another unrelated change.
>
>> if (ep->ep.desc)
>> proc_ep_show(s, ep);
>> }
>
>
> [ snip ]
Thanks for pointing out, I'll remove the changes here and note them down
to send them separately.
>
>> diff --git a/drivers/usb/gadget/udc/net2272.c
>> b/drivers/usb/gadget/udc/net2272.c
>> index 7c38057dcb4a..bb59200f1596 100644
>> --- a/drivers/usb/gadget/udc/net2272.c
>> +++ b/drivers/usb/gadget/udc/net2272.c
>> @@ -926,7 +926,8 @@ static int
>> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> {
>> struct net2272_ep *ep;
>> -struct net2272_request *req;
>> +struct net2272_request *req = NULL;
>> +struct net2272_request *tmp;
>> unsigned long flags;
>> int stopped;
>>
>> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
>> *_req)
>> ep->stopped = 1;
>>
>> /* make sure it's still queued on this endpoint */
>> -list_for_each_entry(req, >queue, queue) {
>> -if (>req == _req)
>> +list_for_each_entry(tmp, >queue, queue) {
>> +if (>req == _req) {
>> +req = tmp;
>> break;
>> +}
>> }
>> -if (>req != _req) {
>> +if (!req) {
>> ep->stopped = stopped;
>> spin_unlock_irqrestore(>dev->lock, flags);
>> return -EINVAL;
>> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
>> *_req)
>> dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
>> net2272_done(ep, req, -ECONNRESET);
>> }
>> -req = NULL;
>
> Another unrelated change. These are all good changes but send them as
> separate patches.
You are referring to the req = NULL, right?
I've changed the use of 'req' in the same function and assumed that I can
just remove the unnecessary statement. But if it's better to do separately
I'll do that.
>
>> ep->stopped = stopped;
>>
>> spin_unlock_irqrestore(>dev->lock, flags);
>
> regards,
> dan carpenter
thanks,
Jakob Koschel
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
From: Linus Torvalds > Sent: 28 February 2022 19:56 > On Mon, Feb 28, 2022 at 4:19 AM Christian König > wrote: > > > > I don't think that using the extra variable makes the code in any way > > more reliable or easier to read. > > So I think the next step is to do the attached patch (which requires > that "-std=gnu11" that was discussed in the original thread). > > That will guarantee that the 'pos' parameter of list_for_each_entry() > is only updated INSIDE the for_each_list_entry() loop, and can never > point to the (wrongly typed) head entry. > > And I would actually hope that it should actually cause compiler > warnings about possibly uninitialized variables if people then use the > 'pos' pointer outside the loop. Except > > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for > inexplicable reasons - possibly because it already expected this > behavior > > (b) when I remove that NULL initializer, I still don't get a warning, > because we've disabled -Wno-maybe-uninitialized since it results in so > many false positives. > > Oh well. > > Anyway, give this patch a look, and at least if it's expanded to do > "(pos) = NULL" in the entry statement for the for-loop, it will avoid > the HEAD type confusion that Jakob is working on. And I think in a > cleaner way than the horrid games he plays. > > (But it won't avoid possible CPU speculation of such type confusion. > That, in my opinion, is a completely different issue) > > I do wish we could actually poison the 'pos' value after the loop > somehow - but clearly the "might be uninitialized" I was hoping for > isn't the way to do it. > > Anybody have any ideas? > > Linus diff --git a/include/linux/list.h b/include/linux/list.h index dd6c2041d09c..bab995596aaa 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -634,10 +634,10 @@ static inline void list_splice_tail_init(struct list_head *list, * @head: the head for your list. * @member:the name of the list_head within the struct. */ -#define list_for_each_entry(pos, head, member) \ - for (pos = list_first_entry(head, typeof(*pos), member);\ -!list_entry_is_head(pos, head, member);\ -pos = list_next_entry(pos, member)) +#define list_for_each_entry(pos, head, member) \ + for (typeof(pos) __iter = list_first_entry(head, typeof(*pos), member); \ +!list_entry_is_head(__iter, head, member) && (((pos)=__iter),1); \ +__iter = list_next_entry(__iter, member)) /** * list_for_each_entry_reverse - iterate backwards over list of given type. I think you actually want: !list_entry_is_head(__iter, head, member) ? (((pos)=__iter),1) : (((pos) = NULL),0); Which can be done in the original by: !list_entry_is_head(pos, head, member) ? 1 : (((pos) = NULL), 0); Although it would be safer to have (without looking up the actual name): for (item *__item = head; \ __item ? (((pos) = list_item(__item, member)), 1) : (((pos) = NULL), 0); __item = (pos)->member) The local does need 'fixing' to avoid shadowing for nested loops. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.
Signed-off-by: Jakob Koschel
---
arch/x86/kernel/cpu/sgx/encl.c | 6 +++--
drivers/scsi/scsi_transport_sas.c| 17 -
drivers/thermal/thermal_core.c | 38 ++--
drivers/usb/gadget/configfs.c| 22 ++--
drivers/usb/gadget/udc/max3420_udc.c | 11 +---
drivers/usb/gadget/udc/tegra-xudc.c | 11 +---
drivers/usb/mtu3/mtu3_gadget.c | 11 +---
drivers/usb/musb/musb_gadget.c | 11 +---
drivers/vfio/mdev/mdev_core.c| 11 +---
9 files changed, 88 insertions(+), 50 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 48afe96ae0f0..6c916416decc 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -450,7 +450,8 @@ static void sgx_mmu_notifier_release(struct mmu_notifier
*mn,
struct mm_struct *mm)
{
struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm,
mmu_notifier);
- struct sgx_encl_mm *tmp = NULL;
+ struct sgx_encl_mm *found_encl_mm = NULL;
+ struct sgx_encl_mm *tmp;
/*
* The enclave itself can remove encl_mm. Note, objects can't be moved
@@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier
*mn,
list_for_each_entry(tmp, _mm->encl->mm_list, list) {
if (tmp == encl_mm) {
list_del_rcu(_mm->list);
+ found_encl_mm = tmp;
break;
}
}
spin_unlock(_mm->encl->mm_lock);
- if (tmp == encl_mm) {
+ if (found_encl_mm) {
synchronize_srcu(_mm->encl->srcu);
mmu_notifier_put(mn);
}
diff --git a/drivers/scsi/scsi_transport_sas.c
b/drivers/scsi/scsi_transport_sas.c
index 4ee578b181da..a8cbd90db9d2 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy);
* connected to a remote device is a port, so ports must be formed on
* all devices with phys if they're connected to anything.
*/
-void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
+void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy)
{
mutex_lock(>phy_list_mutex);
- if (unlikely(!list_empty(>port_siblings))) {
+ if (unlikely(!list_empty(&_phy->port_siblings))) {
/* make sure we're already on this port */
+ struct sas_phy *phy = NULL;
struct sas_phy *tmp;
list_for_each_entry(tmp, >phy_list, port_siblings)
- if (tmp == phy)
+ if (tmp == _phy) {
+ phy = tmp;
break;
+ }
/* If this trips, you added a phy that was already
* part of a different port */
- if (unlikely(tmp != phy)) {
+ if (unlikely(!phy)) {
dev_printk(KERN_ERR, >dev, "trying to add phy %s
fails: it's already part of another port\n",
- dev_name(>dev));
+ dev_name(&_phy->dev));
BUG();
}
} else {
- sas_port_create_link(port, phy);
- list_add_tail(>port_siblings, >phy_list);
+ sas_port_create_link(port, _phy);
+ list_add_tail(&_phy->port_siblings, >phy_list);
port->num_phys++;
}
mutex_unlock(>phy_list_mutex);
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 82654dc8382b..97198543448b 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -625,24 +625,30 @@ int thermal_zone_bind_cooling_device(struct
thermal_zone_device *tz,
{
struct thermal_instance *dev;
struct thermal_instance *pos;
- struct thermal_zone_device *pos1;
- struct thermal_cooling_device *pos2;
+ struct thermal_zone_device *pos1 = NULL;
+ struct thermal_zone_device *tmp1;
+ struct thermal_cooling_device *pos2 = NULL;
+ struct thermal_cooling_device *tmp2;
unsigned long max_state;
int result, ret;
if (trip >= tz->trips || trip < 0)
return -EINVAL;
- list_for_each_entry(pos1, _tz_list, node) {
- if (pos1 == tz)
+ list_for_each_entry(tmp1,
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 21:56 +0100, Christian König wrote:
>
> Am 28.02.22 um 21:42 schrieb James Bottomley:
> > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
> > > Am 28.02.22 um 20:56 schrieb Linus Torvalds:
> > > > On Mon, Feb 28, 2022 at 4:19 AM Christian König
> > > > wrote:
> > > > [SNIP]
> > > > Anybody have any ideas?
> > > I think we should look at the use cases why code is touching
> > > (pos)
> > > after the loop.
> > >
> > > Just from skimming over the patches to change this and experience
> > > with the drivers/subsystems I help to maintain I think the
> > > primary pattern looks something like this:
> > >
> > > list_for_each_entry(entry, head, member) {
> > > if (some_condition_checking(entry))
> > > break;
> > > }
> > > do_something_with(entry);
> >
> > Actually, we usually have a check to see if the loop found
> > anything, but in that case it should something like
> >
> > if (list_entry_is_head(entry, head, member)) {
> > return with error;
> > }
> > do_somethin_with(entry);
> >
> > Suffice? The list_entry_is_head() macro is designed to cope with
> > the bogus entry on head problem.
>
> That will work and is also what people already do.
>
> The key problem is that we let people do the same thing over and
> over again with slightly different implementations.
>
> Out in the wild I've seen at least using a separate variable, using
> a bool to indicate that something was found and just assuming that
> the list has an entry.
>
> The last case is bogus and basically what can break badly.
Yes, I understand that. I'm saying we should replace that bogus checks
of entry->something against some_value loop termination condition with
the list_entry_is_head() macro. That should be a one line and fairly
mechanical change rather than the explosion of code changes we seem to
have in the patch series.
James
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:29 PM Johannes Berg wrote: > > If we're willing to change the API for the macro, we could do > > list_for_each_entry(type, pos, head, member) > > and then actually take advantage of -Wshadow? See my reply to Willy. There is no way -Wshadow will ever happen. I considered that (type, pos, head, member) kind of thing, to the point of trying it for one file, but it ends up as horrendous syntax. It turns out that declaring the type separately really helps, and avoids crazy long lines among other things. It would be unacceptable for another reason too - the amount of churn would just be immense. Every single use of that macro (and related macros) would change, even the ones that really don't need it or want it (ie the good kinds that already only use the variable inside the loop). So "typeof(pos) pos" may be ugly - but it's a very localized ugly. Linus
Re: [Nouveau] [PATCH 3/6] treewide: fix incorrect use to determine if list is empty
On Mon, Feb 28, 2022 at 12:08:19PM +0100, Jakob Koschel wrote: > The list iterator value will *always* be set by list_for_each_entry(). > It is incorrect to assume that the iterator value will be NULL if the > list is empty. > > Instead of checking the pointer it should be checked if > the list is empty. > In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL > on the break, it is set to the correct value and leaving it > NULL if no element was found. > > Signed-off-by: Jakob Koschel > --- > arch/powerpc/sysdev/fsl_gtm.c| 4 ++-- > drivers/media/pci/saa7134/saa7134-alsa.c | 4 ++-- > drivers/perf/xgene_pmu.c | 13 +++-- > 3 files changed, 11 insertions(+), 10 deletions(-) These are all bug fixes. 1) Send them as 3 separate patches. 2) Add Fixes tags. regards, dan carpenter
Re: [Nouveau] [PATCH v2 13/22] drm/nouveau/kms: Remove redundant zpos initialisation
Hi, On Mon, Feb 21, 2022 at 05:42:36PM +0100, Karol Herbst wrote: > On Mon, Feb 21, 2022 at 11:00 AM Maxime Ripard wrote: > > > > The nouveau KMS driver will call drm_plane_create_zpos_property() with > > an init value depending on the plane purpose. > > > > Since the initial value wasn't carried over in the state, the driver had > > to set it again in nv50_wndw_reset(). However, the helpers have been > > adjusted to set it properly at reset, so this is not needed anymore. > > > > Cc: nouveau@lists.freedesktop.org > > Cc: Ben Skeggs > > Cc: Karol Herbst > > Cc: Lyude Paul > > Signed-off-by: Maxime Ripard > > --- > > drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c > > b/drivers/gpu/drm/nouveau/dispnv50/wndw.c > > index 133c8736426a..0c1a2ea0ed04 100644 > > --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c > > +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c > > @@ -635,8 +635,6 @@ nv50_wndw_reset(struct drm_plane *plane) > > plane->funcs->atomic_destroy_state(plane, plane->state); > > > > __drm_atomic_helper_plane_reset(plane, >state); > > - plane->state->zpos = nv50_wndw_zpos_default(plane); > > - plane->state->normalized_zpos = nv50_wndw_zpos_default(plane); > > so reading the surrounding code a little it feels like those > assignments actually do something. If my understanding is correct > plane->state points to >state, but asyw was just kzalloced in > this function. __drm_atomic_helper_plane_reset doesn't set the zpos or > normalized_zpos fields as long as zpos_property is 0, so those fields > won't be set with that change anymore. > > I just don't know if it's fine like that or if this function should > set zpos_property instead or something. Anyway, the commit description > makes it sound like that an unneeded assignment would be removed here, > which doesn't seem to be the case. But I don't really know much about > all the drm API interactions, so it might just be fine, mostly asking > to get a better idea on how all those pieces fit together. If you're looking at the code without that patch series, you're right. These patches change that however: https://lore.kernel.org/dri-devel/20220221095918.18763-7-max...@cerno.tech/ https://lore.kernel.org/dri-devel/20220221095918.18763-8-max...@cerno.tech/ So, once they have been applied those assignments are made in __drm_atomic_helper_plane_reset and are no longer relevant here. Maxime signature.asc Description: PGP signature
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 21:56, Christian König wrote:
>
>
>
> Am 28.02.22 um 21:42 schrieb James Bottomley:
>> On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
>>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
On Mon, Feb 28, 2022 at 4:19 AM Christian König
wrote:
[SNIP]
Anybody have any ideas?
>>> I think we should look at the use cases why code is touching (pos)
>>> after the loop.
>>>
>>> Just from skimming over the patches to change this and experience
>>> with the drivers/subsystems I help to maintain I think the primary
>>> pattern looks something like this:
>>>
>>> list_for_each_entry(entry, head, member) {
>>> if (some_condition_checking(entry))
>>> break;
>>> }
>>> do_something_with(entry);
There are other cases where the list iterator variable is used after the loop
Some examples:
- list_for_each_entry_continue() and list_for_each_entry_from().
- (although very rare) the head is actually of the correct struct type.
(ppc440spe_get_group_entry(): drivers/dma/ppc4xx/adma.c:1436)
- to use pos->list for example for list_add_tail():
(add_static_vm_early(): arch/arm/mm/ioremap.c:107)
If the scope of the list iterator is limited those still need fixing in a
different way.
>>
>> Actually, we usually have a check to see if the loop found anything,
>> but in that case it should something like
>>
>> if (list_entry_is_head(entry, head, member)) {
>> return with error;
>> }
>> do_somethin_with(entry);
>>
>> Suffice? The list_entry_is_head() macro is designed to cope with the
>> bogus entry on head problem.
>
> That will work and is also what people already do.
>
> The key problem is that we let people do the same thing over and over again
> with slightly different implementations.
>
> Out in the wild I've seen at least using a separate variable, using a bool to
> indicate that something was found and just assuming that the list has an
> entry.
>
> The last case is bogus and basically what can break badly.
>
> If we would have an unified macro which search for an entry combined with
> automated reporting on patches to use that macro I think the potential to
> introduce such issues will already go down massively without auditing tons of
> existing code.
Having a unified way to do the same thing would indeed be great.
>
> Regards,
> Christian.
>
>>
>> James
>>
>>
>
- Jakob
[Nouveau] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
If the list representing the request queue does not contain the expected
request, the value of list_for_each_entry() iterator will not point to a
valid structure. To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object.
Signed-off-by: Jakob Koschel
---
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 11 ++
drivers/usb/gadget/udc/at91_udc.c| 26 ++--
drivers/usb/gadget/udc/atmel_usba_udc.c | 11 ++
drivers/usb/gadget/udc/bdc/bdc_ep.c | 11 +++---
drivers/usb/gadget/udc/fsl_qe_udc.c | 11 ++
drivers/usb/gadget/udc/fsl_udc_core.c| 11 ++
drivers/usb/gadget/udc/goku_udc.c| 11 ++
drivers/usb/gadget/udc/gr_udc.c | 11 ++
drivers/usb/gadget/udc/lpc32xx_udc.c | 11 ++
drivers/usb/gadget/udc/mv_u3d_core.c | 11 ++
drivers/usb/gadget/udc/mv_udc_core.c | 11 ++
drivers/usb/gadget/udc/net2272.c | 12 ++-
drivers/usb/gadget/udc/net2280.c | 11 ++
drivers/usb/gadget/udc/omap_udc.c| 11 ++
drivers/usb/gadget/udc/pxa25x_udc.c | 11 ++
drivers/usb/gadget/udc/s3c-hsudc.c | 11 ++
drivers/usb/gadget/udc/udc-xilinx.c | 11 ++
17 files changed, 128 insertions(+), 75 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 917892ca8753..cad874ee4472 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -466,19 +466,22 @@ static int ast_vhub_epn_dequeue(struct usb_ep* u_ep,
struct usb_request *u_req)
{
struct ast_vhub_ep *ep = to_ast_ep(u_ep);
struct ast_vhub *vhub = ep->vhub;
- struct ast_vhub_req *req;
+ struct ast_vhub_req *req = NULL;
+ struct ast_vhub_req *tmp;
unsigned long flags;
int rc = -EINVAL;
spin_lock_irqsave(>lock, flags);
/* Make sure it's actually queued on this endpoint */
- list_for_each_entry (req, >queue, queue) {
- if (>req == u_req)
+ list_for_each_entry(tmp, >queue, queue) {
+ if (>req == u_req) {
+ req = tmp;
break;
+ }
}
- if (>req == u_req) {
+ if (req) {
EPVDBG(ep, "dequeue req @%p active=%d\n",
req, req->active);
if (req->active)
diff --git a/drivers/usb/gadget/udc/at91_udc.c
b/drivers/usb/gadget/udc/at91_udc.c
index 9040a0561466..0fd0307bc07b 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct
at91_ep *ep)
if (list_empty (>queue))
seq_printf(s, "\t(queue empty)\n");
- else list_for_each_entry (req, >queue, queue) {
- unsignedlength = req->req.actual;
+ else
+ list_for_each_entry(req, >queue, queue) {
+ unsigned intlength = req->req.actual;
- seq_printf(s, "\treq %p len %d/%d buf %p\n",
- >req, length,
- req->req.length, req->req.buf);
- }
+ seq_printf(s, "\treq %p len %d/%d buf %p\n",
+ >req, length,
+ req->req.length, req->req.buf);
+ }
spin_unlock_irqrestore(>lock, flags);
}
@@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)
if (udc->enabled && udc->vbus) {
proc_ep_show(s, >ep[0]);
- list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
+ list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
if (ep->ep.desc)
proc_ep_show(s, ep);
}
@@ -704,7 +705,8 @@ static int at91_ep_queue(struct usb_ep *_ep,
static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct at91_ep *ep;
- struct at91_request *req;
+ struct at91_request *req = NULL;
+ struct at91_request *tmp;
unsigned long flags;
struct at91_udc *udc;
@@ -717,11 +719,13 @@ static int at91_ep_dequeue(struct usb_ep *_ep, struct
usb_request *_req)
spin_lock_irqsave(>lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry (req, >queue, queue) {
- if (>req == _req)
+ list_for_each_entry(tmp, >queue, queue) {
+ if (>req == _req) {
+ req = tmp;
break;
+ }
}
- if
Re: [Nouveau] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 01:03:36PM +0100, Jakob Koschel wrote: > >> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request > >> *_req) > >>dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name); > >>net2272_done(ep, req, -ECONNRESET); > >>} > >> - req = NULL; > > > > Another unrelated change. These are all good changes but send them as > > separate patches. > > You are referring to the req = NULL, right? Yes. > > I've changed the use of 'req' in the same function and assumed that I can > just remove the unnecessary statement. But if it's better to do separately > I'll do that. > These are all changes which made me pause during my review to figure out why they were necessary. The line between what is a related part of a patch is a bit vague and some maintainers will ask you to add or subtract from a patch depending on their individual tastes. I don't really have an exact answer, but I felt like this patch needs to be subtracted from. Especially if there is a whole chunk of the patch which can be removed, then to me, that obviously should be in a different patch. regards, dan carpenter
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds wrote: > > We can do > > typeof(pos) pos > > in the 'for ()' loop, and never use __iter at all. > > That means that inside the for-loop, we use a _different_ 'pos' than outside. The thing that makes me throw up in my mouth a bit is that in that typeof(pos) pos the first 'pos' (that we use for just the typeof) is that outer-level 'pos', IOW it's a *different* 'pos' than the second 'pos' in that same declaration that declares the inner level shadowing new 'pos' variable. If I was a compiler person, I would say "Linus, that thing is too ugly to live", and I would hate it. I'm just hoping that even compiler people say "that's *so* ugly it's almost beautiful". Because it does seem to work. It's not pretty, but hey, it's not like our headers are really ever be winning any beauty contests... Linus
[Nouveau] [PATCH 3/6] treewide: fix incorrect use to determine if list is empty
The list iterator value will *always* be set by list_for_each_entry().
It is incorrect to assume that the iterator value will be NULL if the
list is empty.
Instead of checking the pointer it should be checked if
the list is empty.
In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL
on the break, it is set to the correct value and leaving it
NULL if no element was found.
Signed-off-by: Jakob Koschel
---
arch/powerpc/sysdev/fsl_gtm.c| 4 ++--
drivers/media/pci/saa7134/saa7134-alsa.c | 4 ++--
drivers/perf/xgene_pmu.c | 13 +++--
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm.c
index 8963eaffb1b7..39186ad6b3c3 100644
--- a/arch/powerpc/sysdev/fsl_gtm.c
+++ b/arch/powerpc/sysdev/fsl_gtm.c
@@ -86,7 +86,7 @@ static LIST_HEAD(gtms);
*/
struct gtm_timer *gtm_get_timer16(void)
{
- struct gtm *gtm = NULL;
+ struct gtm *gtm;
int i;
list_for_each_entry(gtm, , list_node) {
@@ -103,7 +103,7 @@ struct gtm_timer *gtm_get_timer16(void)
spin_unlock_irq(>lock);
}
- if (gtm)
+ if (!list_empty())
return ERR_PTR(-EBUSY);
return ERR_PTR(-ENODEV);
}
diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c
b/drivers/media/pci/saa7134/saa7134-alsa.c
index fb24d2ed3621..d3cde05a6eba 100644
--- a/drivers/media/pci/saa7134/saa7134-alsa.c
+++ b/drivers/media/pci/saa7134/saa7134-alsa.c
@@ -1214,7 +1214,7 @@ static int alsa_device_exit(struct saa7134_dev *dev)
static int saa7134_alsa_init(void)
{
- struct saa7134_dev *dev = NULL;
+ struct saa7134_dev *dev;
saa7134_dmasound_init = alsa_device_init;
saa7134_dmasound_exit = alsa_device_exit;
@@ -1229,7 +1229,7 @@ static int saa7134_alsa_init(void)
alsa_device_init(dev);
}
- if (dev == NULL)
+ if (list_empty(_devlist))
pr_info("saa7134 ALSA: no saa7134 cards found\n");
return 0;
diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index 2b6d476bd213..e255f9e665d1 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1460,7 +1460,8 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu
*xgene_pmu,
struct hw_pmu_info *inf;
void __iomem *dev_csr;
struct resource res;
- struct resource_entry *rentry;
+ struct resource_entry *rentry = NULL;
+ struct resource_entry *tmp;
int enable_bit;
int rc;
@@ -1475,16 +1476,16 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu
*xgene_pmu,
return NULL;
}
- list_for_each_entry(rentry, _list, node) {
- if (resource_type(rentry->res) == IORESOURCE_MEM) {
- res = *rentry->res;
- rentry = NULL;
+ list_for_each_entry(tmp, _list, node) {
+ if (resource_type(tmp->res) == IORESOURCE_MEM) {
+ res = *tmp->res;
+ rentry = tmp;
break;
}
}
acpi_dev_free_resource_list(_list);
- if (rentry) {
+ if (!rentry) {
dev_err(dev, "PMU type %d: No memory resource found\n", type);
return NULL;
}
--
2.25.1
[Nouveau] [PATCH] drm/nouveau/instmem: fix uninitialized_var.cocci warning
Fix following coccicheck warning: drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c:316:11-12: WARNING this kind of initialization is deprecated. `void *map = map` has the same form of uninitialized_var() macro. I remove the redundant assignement. It has been tested with gcc (Debian 8.3.0-6) 8.3.0. The patch which removed uninitialized_var() is: https://lore.kernel.org/all/20121028102007.ga7...@gmail.com/ And there is very few "/* GCC */" comments in the Linux kernel code now. Signed-off-by: Guo Zhengkui --- drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c index 96aca0edfa3c..c51bac76174c 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c @@ -313,7 +313,7 @@ nv50_instobj_dtor(struct nvkm_memory *memory) struct nv50_instobj *iobj = nv50_instobj(memory); struct nvkm_instmem *imem = >imem->base; struct nvkm_vma *bar; - void *map = map; + void *map; mutex_lock(>mutex); if (likely(iobj->lru.next)) -- 2.20.1
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 11:56 AM Linus Torvalds wrote: > > I do wish we could actually poison the 'pos' value after the loop > somehow - but clearly the "might be uninitialized" I was hoping for > isn't the way to do it. Side note: we do need *some* way to do it. Because if we make that change, and only set it to another pointer when not the head, then we very much change the semantics of "list_for_each_head()". The "list was empty" case would now exit with 'pos' not set at all (or set to NULL if we add that). Or it would be set to the last entry. And regardless, that means that all the if (pos == head) kinds of checks after the loop would be fundamentally broken. Darn. I really hoped for (and naively expected) that we could actually have the compiler warn about the use-after-loop case. That whole "compiler will now complain about bad use" was integral to my clever plan to use the C99 feature of declaring the iterator inside the loop. But my "clever plan" was apparently some ACME-level Wile E. Coyote sh*t. Darn. Linus
Re: [Nouveau] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/usb/gadget/udc/at91_udc.c
> b/drivers/usb/gadget/udc/at91_udc.c
> index 9040a0561466..0fd0307bc07b 100644
> --- a/drivers/usb/gadget/udc/at91_udc.c
> +++ b/drivers/usb/gadget/udc/at91_udc.c
> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct
> at91_ep *ep)
> if (list_empty (>queue))
> seq_printf(s, "\t(queue empty)\n");
>
> - else list_for_each_entry (req, >queue, queue) {
> - unsignedlength = req->req.actual;
> + else
> + list_for_each_entry(req, >queue, queue) {
> + unsigned intlength = req->req.actual;
>
> - seq_printf(s, "\treq %p len %d/%d buf %p\n",
> - >req, length,
> - req->req.length, req->req.buf);
> - }
> + seq_printf(s, "\treq %p len %d/%d buf %p\n",
> + >req, length,
> + req->req.length, req->req.buf);
> + }
Don't make unrelated white space changes. It just makes the patch
harder to review. As you're writing the patch make note of any
additional changes and do them later in a separate patch.
Also a multi-line indent gets curly braces for readability even though
it's not required by C. And then both sides would get curly braces.
> spin_unlock_irqrestore(>lock, flags);
> }
>
> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)
>
> if (udc->enabled && udc->vbus) {
> proc_ep_show(s, >ep[0]);
> - list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
> + list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
Another unrelated change.
> if (ep->ep.desc)
> proc_ep_show(s, ep);
> }
[ snip ]
> diff --git a/drivers/usb/gadget/udc/net2272.c
> b/drivers/usb/gadget/udc/net2272.c
> index 7c38057dcb4a..bb59200f1596 100644
> --- a/drivers/usb/gadget/udc/net2272.c
> +++ b/drivers/usb/gadget/udc/net2272.c
> @@ -926,7 +926,8 @@ static int
> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct net2272_ep *ep;
> - struct net2272_request *req;
> + struct net2272_request *req = NULL;
> + struct net2272_request *tmp;
> unsigned long flags;
> int stopped;
>
> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
> *_req)
> ep->stopped = 1;
>
> /* make sure it's still queued on this endpoint */
> - list_for_each_entry(req, >queue, queue) {
> - if (>req == _req)
> + list_for_each_entry(tmp, >queue, queue) {
> + if (>req == _req) {
> + req = tmp;
> break;
> + }
> }
> - if (>req != _req) {
> + if (!req) {
> ep->stopped = stopped;
> spin_unlock_irqrestore(>dev->lock, flags);
> return -EINVAL;
> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
> *_req)
> dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
> net2272_done(ep, req, -ECONNRESET);
> }
> - req = NULL;
Another unrelated change. These are all good changes but send them as
separate patches.
> ep->stopped = stopped;
>
> spin_unlock_irqrestore(>dev->lock, flags);
regards,
dan carpenter
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote: > We can do > > typeof(pos) pos > > in the 'for ()' loop, and never use __iter at all. > > That means that inside the for-loop, we use a _different_ 'pos' than outside. Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs. > +#define list_for_each_entry(pos, head, member) > \ > + for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); > \ > + !list_entry_is_head(pos, head, member);\ >pos = list_next_entry(pos, member))
[Nouveau] [PATCH 4/6] drivers: remove unnecessary use of list iterator variable
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will *always* be a bogus
pointer computed based on the head element.
To avoid type confusion use the actual list head directly instead of last
iterator value.
Signed-off-by: Jakob Koschel
---
drivers/dma/dw-edma/dw-edma-core.c | 4 ++--
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 3 ++-
drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-edma-core.c
b/drivers/dma/dw-edma/dw-edma-core.c
index 468d1097a1ec..7883c4831857 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -136,7 +136,7 @@ static void dw_edma_free_burst(struct dw_edma_chunk *chunk)
}
/* Remove the list head */
- kfree(child);
+ kfree(chunk->burst);
chunk->burst = NULL;
}
@@ -156,7 +156,7 @@ static void dw_edma_free_chunk(struct dw_edma_desc *desc)
}
/* Remove the list head */
- kfree(child);
+ kfree(desc->chunk);
desc->chunk = NULL;
}
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 091f36adbbe1..c0ea9dbc4ff6 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -3963,7 +3963,8 @@ static void __i40e_reprogram_flex_pit(struct i40e_pf *pf,
* correctly, the hardware will disable flexible field parsing.
*/
if (!list_empty(flex_pit_list))
- last_offset = list_prev_entry(entry, list)->src_offset + 1;
+ last_offset = list_entry(flex_pit_list->prev,
+struct i40e_flex_pit,
list)->src_offset + 1;
for (; i < 3; i++, last_offset++) {
i40e_write_rx_ctl(>hw,
diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
index e3874421c4c0..cf5b05860799 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
@@ -104,7 +104,7 @@ static void ath6kl_credit_init(struct
ath6kl_htc_credit_info *cred_info,
* it use list_for_each_entry_reverse to walk around the whole ep list.
* Therefore assign this lowestpri_ep_dist after walk around the ep_list
*/
- cred_info->lowestpri_ep_dist = cur_ep_dist->list;
+ cred_info->lowestpri_ep_dist = *ep_list;
WARN_ON(cred_info->cur_free_credits <= 0);
--
2.25.1
[Nouveau] [PATCH 5/6] treewide: remove dereference of list iterator after loop body
The list iterator variable will be a bogus pointer if no break was hit.
Dereferencing it could load *any* out-of-bounds/undefined value
making it unsafe to use that in the comparision to determine if the
specific element was found.
This is fixed by using a separate list iterator variable for the loop
and only setting the original variable if a suitable element was found.
Then determing if the element was found is simply checking if the
variable is set.
Signed-off-by: Jakob Koschel
---
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 11 +++
drivers/scsi/wd719x.c | 12
fs/f2fs/segment.c | 9 ++---
3 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
index 57199be082fd..c56cd9e59a66 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
@@ -471,20 +471,23 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx)
static int
nvkm_clk_ustate_update(struct nvkm_clk *clk, int req)
{
- struct nvkm_pstate *pstate;
+ struct nvkm_pstate *pstate = NULL;
+ struct nvkm_pstate *tmp;
int i = 0;
if (!clk->allow_reclock)
return -ENOSYS;
if (req != -1 && req != -2) {
- list_for_each_entry(pstate, >states, head) {
- if (pstate->pstate == req)
+ list_for_each_entry(tmp, >states, head) {
+ if (tmp->pstate == req) {
+ pstate = tmp;
break;
+ }
i++;
}
- if (pstate->pstate != req)
+ if (!pstate)
return -EINVAL;
req = i;
}
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index 1a7947554581..be270ed8e00d 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -684,11 +684,15 @@ static irqreturn_t wd719x_interrupt(int irq, void *dev_id)
case WD719X_INT_SPIDERFAILED:
/* was the cmd completed a direct or SCB command? */
if (regs.bytes.OPC == WD719X_CMD_PROCESS_SCB) {
- struct wd719x_scb *scb;
- list_for_each_entry(scb, >active_scbs, list)
- if (SCB_out == scb->phys)
+ struct wd719x_scb *scb = NULL;
+ struct wd719x_scb *tmp;
+
+ list_for_each_entry(tmp, >active_scbs, list)
+ if (SCB_out == tmp->phys) {
+ scb = tmp;
break;
- if (SCB_out == scb->phys)
+ }
+ if (scb)
wd719x_interrupt_SCB(wd, regs, scb);
else
dev_err(>pdev->dev, "card returned invalid
SCB pointer\n");
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1dabc8244083..a3684385e04a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -356,16 +356,19 @@ void f2fs_drop_inmem_page(struct inode *inode, struct
page *page)
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct list_head *head = >inmem_pages;
struct inmem_pages *cur = NULL;
+ struct inmem_pages *tmp;
f2fs_bug_on(sbi, !page_private_atomic(page));
mutex_lock(>inmem_lock);
- list_for_each_entry(cur, head, list) {
- if (cur->page == page)
+ list_for_each_entry(tmp, head, list) {
+ if (tmp->page == page) {
+ cur = tmp;
break;
+ }
}
- f2fs_bug_on(sbi, list_empty(head) || cur->page != page);
+ f2fs_bug_on(sbi, !cur);
list_del(>list);
mutex_unlock(>inmem_lock);
--
2.25.1
Re: [Nouveau] [PATCH] drm/nouveau: Remove the unused header file nvif/list.h
On 09 2月 22 14:53:19, Cai Huoqing wrote:
> The nouveau driver depends on include/linux/list.h instead of
> nvif/list.h, so remove the obstacle-nvif/list.h.
>
> Signed-off-by: Cai Huoqing
> ---
Ping :)
> drivers/gpu/drm/nouveau/include/nvif/list.h | 353
> 1 file changed, 353 deletions(-)
> delete mode 100644 drivers/gpu/drm/nouveau/include/nvif/list.h
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvif/list.h
> b/drivers/gpu/drm/nouveau/include/nvif/list.h
> deleted file mode 100644
> index 8af5d144ecb0..
> --- a/drivers/gpu/drm/nouveau/include/nvif/list.h
> +++ /dev/null
> @@ -1,353 +0,0 @@
> -/*
> - * Copyright © 2010 Intel Corporation
> - * Copyright © 2010 Francisco Jerez
> - *
> - * Permission is hereby granted, free of charge, to any person obtaining a
> - * copy of this software and associated documentation files (the "Software"),
> - * to deal in the Software without restriction, including without limitation
> - * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> - * and/or sell copies of the Software, and to permit persons to whom the
> - * Software is furnished to do so, subject to the following conditions:
> - *
> - * The above copyright notice and this permission notice (including the next
> - * paragraph) shall be included in all copies or substantial portions of the
> - * Software.
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> - * IN THE SOFTWARE.
> - *
> - */
> -
> -/* Modified by Ben Skeggs to match kernel list APIs */
> -
> -#ifndef _XORG_LIST_H_
> -#define _XORG_LIST_H_
> -
> -/**
> - * @file Classic doubly-link circular list implementation.
> - * For real usage examples of the linked list, see the file test/list.c
> - *
> - * Example:
> - * We need to keep a list of struct foo in the parent struct bar, i.e. what
> - * we want is something like this.
> - *
> - * struct bar {
> - * ...
> - * struct foo *list_of_foos; -> struct foo {}, struct foo {},
> struct foo{}
> - * ...
> - * }
> - *
> - * We need one list head in bar and a list element in all list_of_foos (both
> are of
> - * data type 'struct list_head').
> - *
> - * struct bar {
> - * ...
> - * struct list_head list_of_foos;
> - * ...
> - * }
> - *
> - * struct foo {
> - * ...
> - * struct list_head entry;
> - * ...
> - * }
> - *
> - * Now we initialize the list head:
> - *
> - * struct bar bar;
> - * ...
> - * INIT_LIST_HEAD(_of_foos);
> - *
> - * Then we create the first element and add it to this list:
> - *
> - * struct foo *foo = malloc(...);
> - *
> - * list_add(>entry, _of_foos);
> - *
> - * Repeat the above for each element you want to add to the list. Deleting
> - * works with the element itself.
> - * list_del(>entry);
> - * free(foo);
> - *
> - * Note: calling list_del(_of_foos) will set bar.list_of_foos to an
> empty
> - * list again.
> - *
> - * Looping through the list requires a 'struct foo' as iterator and the
> - * name of the field the subnodes use.
> - *
> - * struct foo *iterator;
> - * list_for_each_entry(iterator, _of_foos, entry) {
> - * if (iterator->something == ...)
> - * ...
> - * }
> - *
> - * Note: You must not call list_del() on the iterator if you continue the
> - * loop. You need to run the safe for-each loop instead:
> - *
> - * struct foo *iterator, *next;
> - * list_for_each_entry_safe(iterator, next, _of_foos, entry) {
> - * if (...)
> - * list_del(>entry);
> - * }
> - *
> - */
> -
> -/**
> - * The linkage struct for list nodes. This struct must be part of your
> - * to-be-linked struct. struct list_head is required for both the head of the
> - * list and for each list node.
> - *
> - * Position and name of the struct list_head field is irrelevant.
> - * There are no requirements that elements of a list are of the same type.
> - * There are no requirements for a list head, any struct list_head can be a
> list
> - * head.
> - */
> -struct list_head {
> -struct list_head *next, *prev;
> -};
> -
> -/**
> - * Initialize the list as an empty list.
> - *
> - * Example:
> - * INIT_LIST_HEAD(>list_of_foos);
> - *
> - * @param The list to initialized.
> - */
> -#define LIST_HEAD_INIT(name) { &(name), &(name) }
> -
> -#define LIST_HEAD(name) \
> - struct list_head name = LIST_HEAD_INIT(name)
> -
> -static inline void
> -INIT_LIST_HEAD(struct list_head *list)
> -{
> -list->next = list->prev = list;
> -}
> -
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote: > On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > > facing" > > > > assumption above. Not having a Thunderbolt spec, I have no idea how > > > > you deal with that. > > > > > > You can download the spec here: [...] > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > > Version 1.0.pdf". > > > > The spec has Host_Router_indication (bits 18-19) as meaning external facing. > > I'll respin the patch 3 for using that. > > Thanks, please include the spec citation when you do. And probably > the URL, because it's not at all obvious how the casual reader would > get from "is_thunderbolt" to a recent add-on to the USB4 spec. PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec, hence there's no connection between "is_thunderbolt" and the USB4 spec. It's a proprietary VSEC used by Intel and the only way to recognize pre-USB4 Thunderbolt devices that I know of. Its ID is also different from the DVSEC IDs given in the above-mentioned spec. Thanks, Lukas
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > [AMD Official Use Only] > > > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > facing" > > > assumption above. Not having a Thunderbolt spec, I have no idea how > > > you deal with that. > > > > You can download the spec here: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > > w.usb.org%2Fsites%2Fdefault%2Ffiles%2FUSB4%2520Specification%2520202 > > 6.zipdata=04%7C01%7Cmario.limonciello%40amd.com%7Ca26e64 > > 7a4acf4e7681d308d9faa358fd%7C3dd8961fe4884e608e11a82d994e183d%7C0 > > %7C0%7C637816402472428689%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC > > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000& > > amp;sdata=HSDqx%2BHzSnczTZxaBij8sgqvJSS8ajtjCzZd2CPSbR4%3Dre > > served=0 > > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > Version 1.0.pdf". > > The spec has Host_Router_indication (bits 18-19) as meaning external facing. > I'll respin the patch 3 for using that. Thanks, please include the spec citation when you do. And probably the URL, because it's not at all obvious how the casual reader would get from "is_thunderbolt" to a recent add-on to the USB4 spec. Bjorn
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
Hi Bjorn, On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-facing" > assumption above. Not having a Thunderbolt spec, I have no idea how > you deal with that. You can download the spec here: https://www.usb.org/sites/default/files/USB4%20Specification%202026.zip Inside the archive there is also the DVSEC spec with name "USB4 DVSEC Version 1.0.pdf".
